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.

Sign up for GitHub

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

Jump to bottom

Parameterized Generators and Pipelines #2132

Merged
merged 166 commits into from
Feb 4, 2021
Merged

Parameterized Generators and Pipelines #2132

merged 166 commits into from
Feb 4, 2021

Conversation

M-Davies
Copy link
Contributor

@M-Davies M-Davies commented Oct 8, 2020 •
edited
Loading

  • Users can now generate their own pipelines using this script from their own repositories to where they want to generate them.
  • Implement repository checkouts to switch between user and adopt files
  • Implement a default params system that allows for more freedom and configuration for the user
  • Relevant doc and styling updates

Closes: #2116
Signed-off-by: Morgan Davies morgandavies2020@gmail.com

@M-Davies M-Davies added enhancement Issues that enhance the code or documentation of the repo in any way regeneration Issues that provide enhancements or fixes to our jenkins job regenerators labels Oct 8, 2020
@M-Davies M-Davies marked this pull request as draft October 8, 2020 14:51
@M-Davies M-Davies force-pushed the parameterised_everything branch 3 times, most recently from 3d24d99 to 2310bfd Compare October 8, 2020 15:44
@karianna karianna added this to the October 2020 milestone Oct 8, 2020
@M-Davies M-Davies force-pushed the parameterised_everything branch 2 times, most recently from e4f7d28 to 27a980f Compare October 9, 2020 12:23
@M-Davies
Copy link
Contributor Author

M-Davies commented Oct 9, 2020

https://ci.adoptopenjdk.net/job/build-scripts/job/utils/job/alpha-build-pipeline-generator/49/console proves this works with custom values and repository (the test jobs were created in wip: https://ci.adoptopenjdk.net/view/work-in-progress/job/build-scripts-testing/). I'll revert the file changes and continue testing

@M-Davies M-Davies force-pushed the parameterised_everything branch 2 times, most recently from fae008a to 1377ad7 Compare October 13, 2020 14:39
@M-Davies M-Davies marked this pull request as ready for review January 29, 2021 22:21
karianna
karianna requested changes Feb 3, 2021
View reviewed changes
Copy link
Contributor

@karianna karianna left a comment

Choose a reason for hiding this comment

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

My Mac is about to restart so saving this very short review, will add more later

FAQ.md Outdated Show resolved Hide resolved
build-farm/set-platform-specific-configurations.sh Outdated Show resolved Hide resolved
build-farm/set-platform-specific-configurations.sh Show resolved Hide resolved
karianna
karianna requested changes Feb 3, 2021
View reviewed changes
Copy link
Contributor

@karianna karianna left a comment

Choose a reason for hiding this comment

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

OK, that's my final review - I think we're close. Once we've proven this works it'll need a serious refactor to remove duplicated code :-)

docs/UsingOurScripts.md Outdated
```md
1. JENKINS PARAMETERS (highest priority, args entered here will be what the build scripts use over everything else)
2. USER JSON (medium priority, args entered here will be used when a jenkins parameter isn't entered)
3. ADOPT JSON (final priority, when jenkins parameters AND a user json arg can't be validated, the script will checkout to this repository and use our defaults json (linked above))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the right order? I'd have thought that the user over ride would be the last one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

User override should ideally be the first otherwise the scripts would just pull in Adopt's data no questions asked. Working vice versa would defeat the point of the PR and Adam's review above.

If a user doesn't want to use Adopt's data and wants to use their own, they should follow the instructions in the guide and either point their defaults.json at whatever the filepath to their data is and leave the Jenkins parameters alone OR stick the path in the Jenkins parameters and leave the defaults.json alone.

Vice versa, if a user wants to use Adopt's data and not have the jobs detect their own data (if they have it), they should remove or invalidate the corresponding entry in their defaults.json and the scripts will automatically pull and use Adopt's scripts.

Happy to discuss this further or validate any points that are confusing? I recognise it's a lot of changes and a 180 degree behaviour in terms of the pipelines 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's have a call - I get what the intention is here, it just goes slightly against the typical design for these sorts of systems where the defaults are laid ontop of each other (and are well documented and disseminated!) and the user overrides come last.

Copy link
Contributor Author

@M-Davies M-Davies Feb 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The final decision has been to open up a follow up issue to address expanding the current USE_ADOPT_SHELL_SCRIPTS parameter to cover all instances of the checking out (see checklist comment).

This will likely require refactoring and upgrading once the repo strip out goes through so we will need to keep that in mind after this is merged

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/AdoptOpenJDK/openjdk-build/issues/2449

docs/UsingOurScripts.md Outdated Show resolved Hide resolved
pipelines/build/common/config_regeneration.groovy Outdated Show resolved Hide resolved
pipelines/build/common/config_regeneration.groovy Outdated Show resolved Hide resolved
pipelines/build/common/create_job_from_template.groovy Outdated Show resolved Hide resolved
pipelines/build/common/kick_off_build.groovy Show resolved Hide resolved
pipelines/build/openjdk10_pipeline.groovy Show resolved Hide resolved
pipelines/build/prTester/kick_off_tester.groovy Outdated Show resolved Hide resolved
pipelines/build/regeneration/build_pipeline_generator.groovy Show resolved Hide resolved
pipelines/build/regeneration/jdk14_regeneration_pipeline.groovy Show resolved Hide resolved
@M-Davies
Copy link
Contributor Author

M-Davies commented Feb 3, 2021

AQA meeting recording of me discussing these changes (start at 3:05) -> https://youtu.be/Fql05uEF9ZA?t=185

@karianna
Copy link
Contributor

karianna commented Feb 3, 2021

I'm afraid a rebase will also be required now sorry!

@smlambert
Copy link
Contributor

Thanks for taking this on @M-Davies (and for walking us through it, re: #2132 (comment)).

@karianna karianna merged commit 485a977 into adoptium:master Feb 4, 2021
@M-Davies
Copy link
Contributor Author

M-Davies commented Feb 4, 2021

Update thread for addressing issues and the checklist comment above -> https://adoptopenjdk.slack.com/archives/C09NW3L2J/p1612435163113400

@M-Davies M-Davies mentioned this pull request Feb 4, 2021
Merged
This was referenced Feb 12, 2021
Open
Open
@M-Davies M-Davies mentioned this pull request Feb 19, 2021
Open
@M-Davies M-Davies deleted the parameterised_everything branch April 30, 2021 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AdamBrousseau AdamBrousseau AdamBrousseau requested changes

@smlambert smlambert smlambert approved these changes

@karianna karianna karianna approved these changes

@sxa sxa Awaiting requested review from sxa

@gdams gdams Awaiting requested review from gdams

Assignees
No one assigned
Labels
documentation Issues that request updates to our documentation enhancement Issues that enhance the code or documentation of the repo in any way refactoring Issues that focus on moving around the codebase inside or between repos regeneration Issues that provide enhancements or fixes to our jenkins job regenerators testing Issues that enhance or fix our test suites
Projects
None yet
Milestone
December 2020
Development

Successfully merging this pull request may close these issues.

Allow regenerators to use custom configs from other repositories
6 participants
@M-Davies @adoptopenjdk-github-bot @smlambert @karianna @AdamBrousseau @sxa