-
-
Notifications
You must be signed in to change notification settings - Fork 71
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
Correct by platform archive AQAvitTapFiles.tar.gz location #1054
Conversation
Signed-off-by: Andrew Leonard <[email protected]>
Thank you for creating a pull request!Please check out the information below if you have not made a pull request here before (or if you need a reminder how things work). Code Quality and Contributing GuidelinesIf you have not done so already, please familiarise yourself with our Contributing Guidelines and Code Of Conduct, even if you have contributed before. TestsGithub actions will run a set of jobs against your PR that will lint and unit test your changes. Keep an eye out for the results from these on the latest commit you submitted. For more information, please see our testing documentation. In order to run the advanced pipeline tests (executing a set of mock pipelines), it requires an admin to post |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these changes imply that we now have many artifacts instead of a single one? (which then needs to be accounted for in later verification and upload steps? I think I missed seeing the initial change, that has the trickle down effect (requires changes in other areas). Guessing this is what @sophia-guo was referring to, regarding verification scripts, requiring updates to handle this new scenario.
@smlambert @sophia-guo Ok, so I was wondering if this looked right!
Is that correct or should we just have (2) ? |
I like them organized by platform, but then the final requirements are that:
|
|
Test with this PR: https://ci.adoptium.net/job/build-scripts/job/openjdk21-pipeline/292/ |
Andrew, to be clear, there is no expectation that we can produce the single tar file, verify it and upload it as a direct part of the release pipeline. It will have to be triggered later, due to the fact that on average we have 20+ Grinder TAP files to include (due to infra or other issues during a release). |
Looks good |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
This is not correct. The reason is the revert changes #1040 is auto or forcefully merged into taps 6e22834 by hit some button of github issues ( not by the PR author). Feels like some new features of github, reviewers has the permission? Those per platform archive are unnecessary and duplicate. Should be deleted. Archives are done by https://github.com/andrew-m-leonard/ci-jenkins-pipelines/blob/8cc041a8c8e96c5c48a984f972bfd642b78f33d3/pipelines/build/common/build_base_file.groovy#L1014-L1040 See PR #1058 |
The "by platform" AQAvitTapFiles.tar.gz archiveArtifact location is wrong, so is not finding it:
When I fixed the the platform AQAvitTapFiles.tar.gz’ location, I forgot to change the achived artifact target!