-
Notifications
You must be signed in to change notification settings - Fork 304
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
HPCC-32690 Add CentOS 7 back into the 9.8.x releases #19133
HPCC-32690 Add CentOS 7 back into the 9.8.x releases #19133
Conversation
@cloLN - FYI |
Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-32690 Jirabot Action Result: |
a6e0a06
to
3ad76f9
Compare
Signed-off-by: Gordon Smith <[email protected]>
3ad76f9
to
e4a02eb
Compare
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 reasonable. One change isn't clear
@@ -112,7 +112,7 @@ jobs: | |||
vcpkg_sha_short=$(git rev-parse --short=8 HEAD) | |||
echo "vcpkg_sha_short=$vcpkg_sha_short" >> $GITHUB_OUTPUT | |||
docker_build_label=hpccsystems/platform-build-base-${{ inputs.os }} | |||
echo "docker_tag=$docker_build_label:$vcpkg_sha_short" >> $GITHUB_OUTPUT | |||
echo "docker_tag=$docker_build_label:${{ inputs.os == 'centos-7' && 'hpcc-platform-9.8.x' || '$vcpkg_sha_short' }}" >> $GITHUB_OUTPUT |
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.
Not clear why this changed - it would be worth having a comment in the file to explain.
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.
Because we have to pin that label to the older 9.8.x build. If he kept the logic the same as everything else, we'd be reliant on a build with the current vcpkg_sha_short. And the current vcpkg version that we use for 9.8.x will fail for centos-7. This looks like a pretty clean way to sidestep that problem and 'pin' the centos-7 builds to our older hpcc-platform-9.8.x tag that we have pushed.
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.
Further - this pattern x == y && aaa || bbb
can be read as x == y ? aaa : bbb
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.
@GordonSmith I was looking for a comment in the yml file explaining why this needs to be special-cased, not what it was doing.
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.
@GordonSmith everything looks good. Pinning it to the hpccsystems/platform-build-base-centos-7:hpcc-platform-9.8.x is how I've been doing the manual builds as well.
@GordonSmith can you add a comment into the file to explain the change highlighted above. |
@GordonSmith this is still waiting for a comment in the file. |
This cannot be merged until the caching problem is resolved. |
Closing because currently done on demand by hand. Reopen if there is a valid PR to introduce this. |
Type of change:
Checklist:
Smoketest:
Testing: