Skip to content

✨ feat: new validator to check Multiple Architectures configuration #233

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
Merged
merged 2 commits into from
Jun 24, 2022
Merged

✨ feat: new validator to check Multiple Architectures configuration #233

merged 2 commits into from
Jun 24, 2022
+1,175 −0

Conversation

camilamacedo86
Copy link
Contributor

Description
add: new validator to check Multiarch configuration

Motivation
Help users, cis and tools verify the Multiarch configuration

@camilamacedo86 camilamacedo86 changed the title add: new validator to check Multiarch configuration WIP: add: new validator to check Multiarch configuration Mar 18, 2022
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 18, 2022
@camilamacedo86 camilamacedo86 changed the title WIP: add: new validator to check Multiarch configuration add: new validator to check Multiarch configuration Mar 18, 2022
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 18, 2022
@camilamacedo86 camilamacedo86 changed the title add: new validator to check Multiarch configuration WIP add: new validator to check Multiarch configuration Mar 24, 2022
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 24, 2022
@camilamacedo86 camilamacedo86 changed the title WIP add: new validator to check Multiarch configuration Add: new validator to check Multiple Architectures configuration Mar 25, 2022
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 25, 2022
ryantking
ryantking suggested changes Mar 25, 2022
View reviewed changes
Copy link
Contributor

@ryantking ryantking left a comment

Choose a reason for hiding this comment

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

Few changes and questions. A lot of it is stylistic stuff to make the code more readable.

@camilamacedo86 camilamacedo86 changed the title Add: new validator to check Multiple Architectures configuration ✨ Add: new validator to check Multiple Architectures configuration Mar 25, 2022
@camilamacedo86 camilamacedo86 changed the title ✨ Add: new validator to check Multiple Architectures configuration ✨ feat: new validator to check Multiple Architectures configuration Mar 25, 2022
@camilamacedo86
Copy link
Contributor Author

Hi @ryantking,

I think we have all addressed this now.
Thank you for the review 🥇
It shows good to fly :-)

jmrodri
jmrodri requested changes Mar 30, 2022
View reviewed changes
Copy link
Member

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

A couple suggestions and question about SO vs OS.

@camilamacedo86
Copy link
Contributor Author

@jmrodri THANK YOU a lot.
All addressed, please feel free to review it again.

@ryantking
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 30, 2022
@camilamacedo86 camilamacedo86 requested a review from jmrodri March 30, 2022 19:28
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 31, 2022
@camilamacedo86 camilamacedo86 reopened this Apr 1, 2022
exdx
exdx reviewed Apr 5, 2022
View reviewed changes
Copy link
Member

@exdx exdx left a comment

Choose a reason for hiding this comment

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

just small comments.

/lgtm

pkg/validation/internal/multiarch.go

// loadInfraLabelsFromCSV will gather the respective labels from the CSV
func (data *multiArchValidator) loadInfraLabelsFromCSV() {
for k, v := range data.bundle.CSV.ObjectMeta.Labels {
Copy link
Member

Choose a reason for hiding this comment

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

would this iteration over the map panic if there are no labels set on the CSV?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was a good catcher. But the label is Labels map[string]string.
So that do not panic if be nil. See: https://go.dev/play/p/VUZPduMAOIf

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 20, 2022
@camilamacedo86 camilamacedo86 requested review from exdx and ryantking June 21, 2022 08:57
@camilamacedo86 camilamacedo86 requested a review from bentito June 21, 2022 13:47
camilamacedo86 and others added 2 commits June 21, 2022 14:50
@camilamacedo86
add: new validator to check MultiArchitetures configuration

Verified

This commit was signed with the committer’s verified signature.
st0012 Stan Lo
GPG key ID: 76D63D5E36F02747
Verified
Learn about vigilant mode
a7f28d7
@camilamacedo86 @bentito
Update pkg/validation/internal/multiarch.go
00581d0 
Co-authored-by: Brett Tofel <btofel@redhat.com>
@camilamacedo86 camilamacedo86 requested a review from bentito June 21, 2022 13:55
@camilamacedo86 camilamacedo86 requested a review from bentito June 21, 2022 14:26
@bentito
Copy link
Contributor

bentito commented Jun 21, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 21, 2022
dinhxuanvu
dinhxuanvu approved these changes Jun 24, 2022
View reviewed changes
Copy link
Member

@dinhxuanvu dinhxuanvu left a comment

Choose a reason for hiding this comment

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

/approve

@openshift-ci
Copy link

openshift-ci bot commented Jun 24, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, dinhxuanvu

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 24, 2022
@openshift-ci openshift-ci bot merged commit decf748 into operator-framework:master Jun 24, 2022
@camilamacedo86 camilamacedo86 deleted the add-multi-arch branch June 24, 2022 13:38
timflannagan added a commit to timflannagan/operator-framework-olm that referenced this pull request Jul 26, 2022
@timflannagan
Update the o-f/api denylist commits
953de99 
Add the commit from operator-framework/api#233
that requires the 'docker' executable to be present in order to test
code paths added in that PR. Downstream CI doesn't have docker available
in the CI containers, and therefore, CI will fail when running the
staging API unit tests if this commit is present.

Signed-off-by: timflannagan <timflannagan@gmail.com>
timflannagan added a commit to timflannagan/operator-framework-olm that referenced this pull request Jul 27, 2022
@timflannagan
Update the o-f/api denylist commits
a71cca2 
Add the commit from operator-framework/api#233
that requires the 'docker' executable to be present in order to test
code paths added in that PR. Downstream CI doesn't have docker available
in the CI containers, and therefore, CI will fail when running the
staging API unit tests if this commit is present.

Signed-off-by: timflannagan <timflannagan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dinhxuanvu dinhxuanvu

@ankitathomas ankitathomas

@jmrodri jmrodri

@exdx exdx

@ryantking ryantking

@perdasilva perdasilva

@bentito bentito

Assignees

@ryantking ryantking

@bentito bentito

@exdx exdx

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@camilamacedo86 @ryantking @bentito @jmrodri @perdasilva @dinhxuanvu @exdx