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): pattern matching for target defaults #26870

Merged
merged 2 commits into from
Jul 12, 2024

Conversation

AgentEnder
Copy link
Member

Current Behavior

TargetDefaults can be specified by keys matching either executor or target name only

Expected Behavior

TargetDefaults can match based on a glob pattern that may match the target name. This is useful for things like e2e-ci--*. Only 1 target default will ever apply to a given target. We recognize this may be confusing, but is inline with current handling. If no default matches the target name or key, the first default that matches by pattern will be used.

Related Issue(s)

Fixes #

@AgentEnder AgentEnder requested a review from a team as a code owner July 9, 2024 14:29
@AgentEnder AgentEnder requested a review from Cammisuli July 9, 2024 14:29
Copy link

vercel bot commented Jul 9, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Updated (UTC)
nx-dev ⬜️ Ignored (Inspect) Visit Preview Jul 11, 2024 10:16pm

Copy link

nx-cloud bot commented Jul 9, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 4a01bc9. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@AgentEnder AgentEnder force-pushed the feat/pattern-matching-targets branch 3 times, most recently from 7111249 to 840f448 Compare July 9, 2024 15:33
Comment on lines 1045 to 1055
for (const key in targetDefaults ?? {}) {
if (isGlobPattern(key) && minimatch(targetName, key)) {
return targetDefaults[key];
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change this to find the pattern which is the longest character length wise.

@AgentEnder AgentEnder force-pushed the feat/pattern-matching-targets branch from 840f448 to 4a01bc9 Compare July 11, 2024 22:13
@@ -2,3 +2,14 @@ export function combineGlobPatterns(...patterns: (string | string[])[]) {
const p = patterns.flat();
return p.length > 1 ? '{' + p.join(',') + '}' : p.length === 1 ? p[0] : '';
}

export const GLOB_CHARACTERS = new Set(['*', '|', '{', '}', '(', ')', '[']);
Copy link
Member

Choose a reason for hiding this comment

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

These are all the characters I found when doing the whole glob conversion stuff in the Rust side. Are they needed here as well (like the ?, @, etc)?

pub(crate) fn contains_glob_pattern(value: &str) -> bool {
value.contains('!')
|| value.contains('?')
|| value.contains('@')
|| value.contains('+')
|| value.contains('*')
|| value.contains('|')
|| value.contains(',')
|| value.contains('{')
|| value.contains('}')
|| value.contains('[')
|| value.contains(']')
|| value.contains('(')
|| value.contains(')')
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so... ?, @, +, ! are only valid in combination with paraenthises

@AgentEnder AgentEnder requested a review from FrozenPandaz July 12, 2024 14:23
@FrozenPandaz FrozenPandaz merged commit 92d0e15 into master Jul 12, 2024
6 checks passed
@FrozenPandaz FrozenPandaz deleted the feat/pattern-matching-targets branch July 12, 2024 20:25
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 Jul 19, 2024
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.

3 participants