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

Ensure RHEL tests use a valid version of Management Center [DI-422] #880

Merged
merged 12 commits into from
Feb 24, 2025

Conversation

JackPGreen
Copy link
Collaborator

@JackPGreen JackPGreen commented Feb 6, 2025

The RHEL publish tests the build by connecting an Management Center instance to it.

In #847 this was changed to use a matching version of Hazelcast & Management Center - which isn't correct in all cases (e.g. 5.5.2 Hazelcast & 5.6.0 Management Center)

Before this change, it used the latest version of Management Center - derived by inspecting the tags of the (archived) https://github.com/hazelcast/management-center-openshift repo - but the last tag here was 5.3.2 so it wasn't really the latest at all.

In consultation with the Management Center team, latest is the best option.

Changes:

  • rolls back to something inspired by the previous implementation - choses latest based on tags
  • tags are scraped from the hazelcast/management-center repo
  • tags are queried using the gh cli tool to avoid checking out the whole repo

I don't think we should backport this change, as while it's not correct in earlier versions, it doesn't cause any issues.

Fixes: DI-422

The RHEL publish tests the build by connecting an Management Center instance to it.

In #847 this was changed to use a _matching_ version of Hazelcast & Management Center - which isn't correct in all cases (e.g. `5.5.2` Hazelcast & `5.6.0` Management Center)

Before this change, it used the latest version of Management Center - derived by inspecting the tags of the (archived) https://github.com/hazelcast/management-center-openshift repo - but the last tag here was `5.3.2` so it wasn't really the latest at all.

In [consultation with the Management Center team](https://hazelcast.slack.com/archives/C9S7BV74Z/p1738850975557879), latest is the best option.

Changes:
- choses latest based on tags
- tags are scraped from the `hazelcast/management-center` repo
- tags are queried using the `gh` cli tool to avoid checking out the whole repo

I don't think we should backport this change, as while it's not correct in earlier versions, it doesn't cause an issues.

Fixes: [DI-422](https://hazelcast.atlassian.net/browse/DI-422)
@JackPGreen JackPGreen self-assigned this Feb 6, 2025
@JackPGreen JackPGreen requested a review from a team as a code owner February 6, 2025 14:48
@nishaatr
Copy link
Contributor

nishaatr commented Feb 7, 2025

Just had a thought about sorting. Might not work with double digits e.g. v5.5.32
Not sure tbh but has cropped up before
MC tend to do more releases so there is chance this might happen so worth testing
you could use sort -V or something similar.

@JackPGreen
Copy link
Collaborator Author

Just had a thought about sorting. Might not work with double digits e.g. v5.5.32 Not sure tbh but has cropped up before MC tend to do more releases so there is chance this might happen so worth testing you could use sort -V or something similar.

The tags are returned in date order. I've taken the sort out to avoid adding any extra logic there.

@JackPGreen
Copy link
Collaborator Author

Just had a thought about sorting. Might not work with double digits e.g. v5.5.32 Not sure tbh but has cropped up before MC tend to do more releases so there is chance this might happen so worth testing you could use sort -V or something similar.

The tags are returned in date order. I've taken the sort out to avoid adding any extra logic there.

Actually changed my mind - if we do a patch release of (say) 5.1.1, that shouldn't be considered better to use than (say) 5.6.0. Now uses the sort as you suggested.

374765e

@nishaatr
Copy link
Contributor

nishaatr commented Feb 7, 2025

Actually changed my mind - if we do a patch release of (say) 5.1.1, that shouldn't be considered better to use than (say) 5.6.0. Now uses the sort as you suggested.

Sorry where in the regex test("^v\\d+(\\.\\d+)*$")) is this done i.e. should it not end with .0?

I guess we can limit to major/minor but not sure why we should not consider PATCH. Any good reason?
This is just for internal testing so perhaps MANOR/MINOR should be fine

@JackPGreen
Copy link
Collaborator Author

Actually changed my mind - if we do a patch release of (say) 5.1.1, that shouldn't be considered better to use than (say) 5.6.0. Now uses the sort as you suggested.

Sorry where in the regex test("^v\\d+(\\.\\d+)*$")) is this done i.e. should it not end with .0?

I guess we can limit to major/minor but not sure why we should not consider PATCH. Any good reason?

This is just for internal testing so perhaps MANOR/MINOR should be fine

It's not checking for 0 - that was just an example of the kind of scenario where it could've gone wrong.

I was trusting GitHub returning the most recent tag would be the latest version - but when we say latest we really mean highest - so added an explicit semver-aware sort to assert that.

@JackPGreen
Copy link
Collaborator Author

@ldziedziul are you able to review, please?

--paginate \
--jq '.[] | select(.name | test("^v\\d+(\\.\\d+)*$")) | .name')
tags_sorted=$(sort --version-sort --reverse <<< "${tags}")
head --lines=1 <<< "${tags_sorted}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why <<< instead of |?

Copy link
Collaborator Author

@JackPGreen JackPGreen Feb 17, 2025

Choose a reason for hiding this comment

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

I wanted to store as a temp variable for debug / set -x failure issues.
And I thought that <<< (here strings) are better than echo $tags_sorted | head - I seem to remember working around some JSON escaping issue this way in the past.

@JackPGreen JackPGreen enabled auto-merge (squash) February 24, 2025 16:39
@JackPGreen JackPGreen merged commit ca0ab60 into master Feb 24, 2025
16 checks passed
@JackPGreen JackPGreen deleted the DI-422 branch February 24, 2025 16:43
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.

3 participants