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

feat(core): exclude projects from affected based on tags #3812

Closed
wants to merge 4 commits into from

Conversation

yharaskrik
Copy link
Contributor

Current Behavior

There is no current way to exclude projects from affected:* commands based on tags (only explicitly with project name).

Expected Behavior

Should be able to pass in a csv list of tags to exclude from affected commands

yarn nx affected:libs --excludeTags="scope:noop"

Any libraries with scope:noop in their tags array in nx.json should not be considered affected

@yharaskrik
Copy link
Contributor Author

An extension I could make on this if there is interest is to add an onlyTags option that allows the user to specify what projects to run based on tags from the affected list.

Would have to discuss what would take precedent.

@nx-cloud
Copy link

nx-cloud bot commented Sep 25, 2020

Nx Cloud Report

CI ran the following commands for commit 9e18a6c. Click to see the status, the terminal output, and the build insights.

Status Command Start Time
#000000 nx run-many --target=build --all --parallel 9/30/2020, 1:06:07 PM
#000000 nx run-many --target=e2e --projects=e2e-workspace,e2e-cli 9/30/2020, 1:06:15 PM

Sent with 💌 from NxCloud.

@@ -279,6 +279,13 @@ function withAffectedOptions(yargs: yargs.Argv): yargs.Argv {
.option('verbose', {
describe: 'Print additional error stack trace on failure',
})
.option('excludeTags', {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

  • add a default value of []
  • consider moving this below the existing 'exclude' option so it's immediately relatable when someone new is going through the nx code, like so:
    .option('exclude', {
      describe: 'Exclude certain projects from being processed',
      type: 'array',
      coerce: parseCSV,
      default: [],
    })
    .option('excludeTags', {
      describe:
        'Exclude projects with tags matching one or more of the provided tags',
      type: 'array',
      requiresArg: false,
      coerce: parseCSV,
      default: [],
    })

PS: I'm not an nx team member, so please feel free to ignore my suggestion if you disagree.
I liked your PR idea and was going through the changes and wanted to share my 2 cents. ✌🏼

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually had that in one of my prior commits but wasn't sure if it was fully necessary as it affected some tests (because of the default value) that weren't really relevant (ex. some tests failed cause it was expecting the default for excludeTags but it didn't have a default for other params that accepted a csv list).

Instead, I opted for checking to see if it was an array in the code when it is being used.

Not sure what the best way to go about it is.

Would like to hear what one of the Nx team members thinks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving it close to excludes isn't a bad idea though. Sorry i missed that my first read.

@vsavkin
Copy link
Member

vsavkin commented Oct 29, 2020

@FrozenPandaz it looks like an interesitng idea. Can you review it?

@jeremylvln
Copy link
Contributor

Any news on this? I was just looking for this feature before creating my own PR.

I'm creating a monorepo with multiple languages. I'm looking for separating the projects in multiple CI runners, one per language. Using tags like lang:js or lang:python to split the jobs would be neat!

@yharaskrik
Copy link
Contributor Author

@IamBlueSlime just waiting on @FrozenPandaz to review it. There's a chance ill need to make some changes to it.

@jeremylvln
Copy link
Contributor

@IamBlueSlime just waiting on @FrozenPandaz to review it. There's a chance ill need to make some changes to it.

If there any chance for providing the onlyTags option you were talking in one of your comment above? These two options will cover all the usecases IMO.

Thanks!

@yharaskrik
Copy link
Contributor Author

Either way I'll need to wait a for a review, but yes onlyTags should cover a lot of the use cases. It's up the Nrwl team to decide what they want included.

@AgentEnder
Copy link
Member

This would help greatly in the context of an xplat workspace as well, utilizing platform:{web/nativescript/etc} tags to build certain apps, or exclude demo apps from deployment pipelines.

@@ -1,6 +1,6 @@
{
"name": "@nrwl/nx-source",
"version": "10.2.1",
"version": "999.9.8",
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be checked in

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. I didn't notice this. I'll remove it thanks!

I'm going to take a second look at this anyway and possibly make some changes to it.

@mehrad-rafigh
Copy link
Contributor

mehrad-rafigh commented Jan 25, 2021

This would also close this #1621. Thank your for your PR! ❤️
@yharaskrik Could you please add to your commit as well?

@FrozenPandaz
Copy link
Collaborator

Sorry @yharaskrik I don't think we will be merging this soon. This seems similar to #4557 and we have similar thoughts on that one.

@vsavkin and I have been discussing using --tags for run-many and affected for a while. We don't think this feature is out of the question. However, we're not sure it promotes the best practices. Before this is implemented, I think the discussion should continue to clarify use cases of this feature.

Having worked in many workspaces large and small, we have yet to see a clear case where this feature would be part of a workflow.

It definitely makes sense for a few apps to be excluded while they are still in early stages but that should hopefully change after a short period of time. I'm not sure when a project would be excluded long term but still be relevant enough to keep in the workspace. This makes less sense when all projects of a tag are excluded. It's not clear to me why this group is excluded from Nx but still in the workspace.

Again, before this is implemented, I think the discussion should continue to clarify use cases of this feature. Would it be alright to close this PR for now?

@yharaskrik
Copy link
Contributor Author

That is totally fine @FrozenPandaz, thank you for writing a response up :) I appreciate your time. I totally get it.

@yharaskrik yharaskrik closed this Apr 29, 2021
@yharaskrik
Copy link
Contributor Author

Hey @FrozenPandaz and @vsavkin reviving this a bit as we recently came across a good use case for something like this and I was wondering if you guys wanted someone to ake a stab at a slightly different approach. Use a list of tags to AND affected/run-many. Our use case:

For regulator purposes we need to have an entirely different stack setup in a new environment for testing. This Stack only need a subset of our projects to be deployed. Having a tag for this case would help us affected deploy this sub-stack

Let me know if you have this in the works or if I could take a stab at it!

@github-actions
Copy link

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants