Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Verify no mov-ai duplicates #287

Open
wants to merge 1 commit into
base: v2.1
Choose a base branch
from
Open

Verify no mov-ai duplicates #287

wants to merge 1 commit into from

Conversation

quirinpa
Copy link
Contributor

@quirinpa quirinpa commented Jun 18, 2024

  • FP-2834: Remove duplicated dependencies on all apps by moving peer dependencies (only part of the work for this task)

Copy link
Contributor

@AlexFernandes-MOVAI AlexFernandes-MOVAI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

proposition, just reviewing the code for better readility

@@ -133,6 +133,36 @@ jobs:
npm ci --loglevel verbose
fi

- name: Validate specified deps not duplicated
if: ${{ inputs.custom_deps_not_dup == 'true' }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NPM can be defined as an env var, with

Suggested change
if: ${{ inputs.custom_deps_not_dup == 'true' }}
if: ${{ inputs.custom_deps_not_dup == 'true' }}
env:
NPM: ${{ inputs.pm }}

run: |
NPM=${{ inputs.pm }}

if test "$NPM" = "npm"; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alias may_tail is a working solution but I would prefer to have 2 sets of functions for better readibility:
dupe_check_npm()
dupe_check_other()

each one being called in a dedicated task of the workflow maybe:
- name: Validate specified deps not duplicated with NPM
- name: Validate specified deps not duplicated with other

@duartecoelhomovai duartecoelhomovai requested a review from diasdm June 18, 2024 16:05
@duartecoelhomovai
Copy link
Contributor

I'm not really up to date with what was discussed in frontend channel. @diasdm

@duartecoelhomovai
Copy link
Contributor

Test the pipeline using this branch and please paste it here the run.

Copy link
Contributor

@diasdm diasdm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before merging this we need to fully understand how we'll manage dependencies on EE 2.4.1 apps, which we don't know ATM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants