Skip to content

[JENKINS-64508] Add extension point for globally defined when conditions #409

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

Merged
merged 4 commits into from
Jan 9, 2021

Conversation

abayer
Copy link
Member

@abayer abayer commented Dec 22, 2020

  • JENKINS issue(s):
  • Description:
    • Adds a new extension point and parse/transformation logic for globally defined when conditions that will be evaluated for every stage run on the controller when configured.
    • Also get the debug pretty printing for post-AST transformation to actually do something kinda useful again.
  • Documentation changes:
    • n/a - no user-facing changes
  • Users/aliases to notify:
    • ...

@abayer abayer requested a review from bitwiseman December 22, 2020 16:06

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Also get the debug pretty printing for post-AST transformation to actually do something kinda useful again.

Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
@abayer abayer force-pushed the when-runtime-contributor branch from b7a6bf0 to 599c3f0 Compare December 22, 2020 16:52
Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
@michaelneale
Copy link
Member

neat

/*
* The MIT License
*
* Copyright (c) 2021, CloudBees, Inc.
Copy link
Member

Choose a reason for hiding this comment

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

already 2021? nice.

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’m being proactive since I don’t expect this to merge this year. =)

@abayer abayer marked this pull request as ready for review December 28, 2020 16:56
Comment on lines 112 to 115
int result = super.hashCode();
result = 31 * result + (getStageName() != null ? getStageName().hashCode() : 0);
result = 31 * result + (getStage() != null ? getStage().hashCode() : 0);
return result;
Copy link
Contributor

Choose a reason for hiding this comment

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

Use Objects.hashCode instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Huh, smart!

Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
Copy link
Contributor

@bitwiseman bitwiseman left a comment

Choose a reason for hiding this comment

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

It looks like this doesn't include any syntax changes. Is that correct?

Comment on lines 32 to 34
* When container generated when adding invisible global when conditions to a stage, containing the new conditions and
* any explicitly defined ones. When created with existing conditions, the existing when container is stored for use as
* well. This is used as a marker to avoid validation, JSON/Groovy generation, etc for the generated container.
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs clarification. Maybe at least use Javadoc code markers to indicate when you are talking about when and when you are not.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do.

@@ -321,7 +321,7 @@ class ModelParser implements Parser {

// Lazily evaluate prettyPrint(...) - i.e., only if AST_DEBUG_LOGGING is true.
astDebugLog {
"Transformed runtime AST:\n${ -> prettyPrint(pipelineBlock.whole.arguments)}"
"Transformed runtime AST:\n${ -> prettyPrint(src)}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps you could add a test where astDebugLog is enabled to protect it from future breakage?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea - I'll see what I can do.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is impressively difficult to really test - it needs a ModuleNode passed to it, specifically one that's been generated by RuntimeASTTransformer from a SourceUnit and a parsed ModelASTPipelineDef so that it'll actually look like the real thing. Lemme think on this...

Copy link
Member Author

Choose a reason for hiding this comment

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

...doesn't help that for some reason I can't run tests from intellij right now. heh.

Copy link
Member Author

Choose a reason for hiding this comment

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

woo, got it.

@abayer
Copy link
Member Author

abayer commented Dec 28, 2020

@bitwiseman - yup, no syntax changes of any kind.

Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
@bitwiseman bitwiseman merged commit bcdbb4e into jenkinsci:master Jan 9, 2021
@abayer
Copy link
Member Author

abayer commented Jan 11, 2021

Thanks, @bitwiseman!

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.

None yet

3 participants