MGMT-17503: Fix the cpu architectures retrieved in ../openshift-versions endpoint by danmanor · Pull Request #6184 · openshift/assisted-service

danmanor · 2024-04-14T11:51:53Z

Currently when requesting the latest openshift versions at ../openshift-versions?only_latest=true, you get at most one CPU architecture in the list. This PR fixes that issue.

List all the issues related to this PR

What environments does this code impact?

Automation (CI, tools, etc)
Cloud
Operator Managed Deployments
None

How was this code tested?

assisted-test-infra environment
dev-scripts environment
Reviewer's test appreciated
Waiting for CI to do a full test run
Manual (Elaborate on how it was tested)
No tests needed

Checklist

Title and description added to both, commit and PR.
Relevant issues have been associated (see CONTRIBUTING guide)
This change does not require a documentation update (docstring, docs, README, etc)
Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

Are the title and description (in both PR and commit) meaningful and clear?
Is there a bug required (and linked) for this change?
Should this PR be backported?

openshift-ci-robot · 2024-04-14T11:51:57Z

openshift-ci-robot · 2024-04-14T11:52:11Z

openshift-ci-robot · 2024-04-14T11:52:15Z

codecov · 2024-04-14T12:13:50Z

Codecov Report

Attention: Patch coverage is 81.57895% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 68.59%. Comparing base (4c9b414) to head (f5e3c07).
Report is 10 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6184      +/-   ##
==========================================
+ Coverage   68.22%   68.59%   +0.37%     
==========================================
  Files         242      240       -2     
  Lines       35884    36330     +446     
==========================================
+ Hits        24481    24920     +439     
+ Misses       9276     9241      -35     
- Partials     2127     2169      +42

Files	Coverage Δ
internal/common/version.go	`69.56% <66.66%> (-0.28%)`	⬇️
internal/versions/api.go	`79.61% <84.37%> (-1.97%)`	⬇️

... and 13 files with indirect coverage changes

danmanor · 2024-04-15T14:26:12Z

/test edge-subsystem-aws /test edge-subsystem-kubeapi-aws

danmanor · 2024-04-15T14:26:40Z

/test edge-subsystem-kubeapi-aws
/test edge-subsystem-aws

danmanor · 2024-04-16T16:16:32Z

/retest

danmanor · 2024-04-17T06:48:28Z

/retest

adriengentil · 2024-04-17T08:08:20Z

Suggested change

if *isEqual && !lo.Contains(latestReleaseImages, releaseImage) {

if *isEqual && !funk.Contains(latestReleaseImages, releaseImage) {

funk is already widely used in assisted, let's keep using it instead of introducing other dependencies to perform the same operations.

lo it's strongly typed as opposed as funk which is not, and we introduced it already in some other parts of the code as not all lo operations are supported by funk.
Also we will probably deprecate both once we hit 1.22 (IIRC) which has generic slices operations built in

I had a discussion with @rccrdpccl about it, I think we should start using lo instead, it is better

Also we will probably deprecate both once we hit 1.22 (IIRC) which has generic slices operations built in

I agree, 1.22 will unify all these kind of libraries.

But I think we should avoid introducing new libraries that do almost the same thing than another one, even if that new library is technically better, I think consistency is more important, it avoids switching mentally between two libraries while reviewing. Already in this file, both libraries are in use.
Or we decide to go for the new one, and then we make the work to refactor over it.

lo is currently use only in this file https://github.com/openshift/assisted-service/blob/master/internal/operators/mce/mce_operator.go#L214, and go-funk already provide such functions: https://pkg.go.dev/github.com/thoas/go-funk?utm_source=godoc#Sum

@adriengentil Do you think it worth running the tests again for? I think this is the only issue left

I am also a fan of lo as you don't need to assert the type :)

I think we should follow up on that because my concerns still stand. Once we will start to use go 1.22 we will have 3 different ways to do the same thing in this project.

danmanor · 2024-04-17T13:24:13Z

/test images

…ons endpoint

openshift-ci-robot · 2024-04-17T13:45:12Z

/retest-required

Remaining retests: 0 against base HEAD 29e748f and 2 for PR HEAD d3366b72cc85632e7f4e8dc585f40f8a645aedf7 in total

openshift-ci · 2024-04-17T13:59:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adriengentil, danmanor

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [adriengentil,danmanor]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2024-04-17T14:45:20Z

/retest-required

Remaining retests: 0 against base HEAD 29e748f and 2 for PR HEAD f5e3c07 in total

danmanor · 2024-04-17T18:07:58Z

/retest

danmanor · 2024-04-17T21:25:02Z

/retest

danmanor · 2024-04-17T21:25:24Z

/test e2e-agent-compact-ipv4 images

openshift-ci · 2024-04-18T05:09:23Z

@danmanor: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

danmanor · 2024-04-18T07:10:53Z

/cherry-pick cloud_hotfix_releases

openshift-cherrypick-robot · 2024-04-18T07:11:46Z

@danmanor: new pull request created: #6206

Details

In response to this:

/cherry-pick cloud_hotfix_releases

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Apr 14, 2024

danmanor changed the title ~~MGMT-17503: MGMT-17503: Fix the cpu architectures retrieved in ../openshift-versions endpoint~~ MGMT-17503: Fix the cpu architectures retrieved in ../openshift-versions endpoint Apr 14, 2024

openshift-ci Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 14, 2024

openshift-ci Bot requested review from javipolo and rccrdpccl April 14, 2024 11:53

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 14, 2024

adriengentil reviewed Apr 17, 2024

View reviewed changes

danmanor force-pushed the fix-latest-diffs branch from 9579ea0 to 07cbcb6 Compare April 17, 2024 12:07

danmanor requested a review from adriengentil April 17, 2024 12:37

adriengentil reviewed Apr 17, 2024

View reviewed changes

Comment thread internal/versions/api.go Outdated

adriengentil reviewed Apr 17, 2024

View reviewed changes

Comment thread internal/versions/api.go Outdated

Comment thread internal/versions/api.go Outdated

danmanor force-pushed the fix-latest-diffs branch from 07cbcb6 to d3366b7 Compare April 17, 2024 13:16

danmanor requested a review from adriengentil April 17, 2024 13:16

adriengentil approved these changes Apr 17, 2024

View reviewed changes

openshift-ci Bot assigned adriengentil Apr 17, 2024

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2024

MGMT-17503: Fix the cpu architectures retrieved in ../openshift-versi…

f5e3c07

…ons endpoint

danmanor force-pushed the fix-latest-diffs branch from d3366b7 to f5e3c07 Compare April 17, 2024 13:44

openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2024

adriengentil approved these changes Apr 17, 2024

View reviewed changes

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2024

openshift-merge-bot Bot merged commit dffc2f9 into openshift:master Apr 18, 2024

openshift-cherrypick-robot mentioned this pull request Apr 18, 2024

[cloud_hotfix_releases] MGMT-17503: Fix the cpu architectures retrieved in ../openshift-versions endpoint #6206

Merged

adriengentil mentioned this pull request Jun 5, 2024

oci supported #6405

Closed

	if *isEqual && !lo.Contains(latestReleaseImages, releaseImage) {
	if *isEqual && !funk.Contains(latestReleaseImages, releaseImage) {

Conversation

danmanor commented Apr 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

Uh oh!

openshift-ci-robot commented Apr 14, 2024 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

Uh oh!

openshift-ci-robot commented Apr 14, 2024 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

Uh oh!

openshift-ci-robot commented Apr 14, 2024 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

Uh oh!

codecov Bot commented Apr 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

danmanor commented Apr 15, 2024

Uh oh!

danmanor commented Apr 15, 2024

Uh oh!

danmanor commented Apr 16, 2024

Uh oh!

danmanor commented Apr 17, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danmanor commented Apr 17, 2024

Uh oh!

openshift-ci-robot commented Apr 17, 2024

Uh oh!

openshift-ci Bot commented Apr 17, 2024

Uh oh!

openshift-ci-robot commented Apr 17, 2024

Uh oh!

danmanor commented Apr 17, 2024

danmanor commented Apr 14, 2024 •

edited

Loading

openshift-ci-robot commented Apr 14, 2024 •

edited by openshift-ci Bot

Loading

openshift-ci-robot commented Apr 14, 2024 •

edited by openshift-ci Bot

Loading

openshift-ci-robot commented Apr 14, 2024 •

edited by openshift-ci Bot

Loading

codecov Bot commented Apr 14, 2024 •

edited

Loading