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.

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

adding rpm architecture of to our maps, for 32 bit and 64 bit rpms #1038

Merged
merged 1 commit into from Jul 27, 2023
Merged

adding rpm architecture of to our maps, for 32 bit and 64 bit rpms #1038

merged 1 commit into from Jul 27, 2023

Conversation

acornett21
Copy link
Contributor

@acornett21 acornett21 commented Jul 18, 2023 •
edited

  • Adding pkg.Arch as a key value to our maps so we are comparing files on an arch by arch basis, and so we are not overwriting any map value, if multiple arch's of a given rpm exists in a given layer.
  • Updating installedFileMapWithExclusions method to filter out directories
  • Updating installedFileMapWithExclusions to filter out files from the same rpm if only architecture differs
  • Updating/Adding docker files to test more success/failures of the HasModifiedFiles check

@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 Jul 18, 2023
@openshift-ci
Copy link

openshift-ci bot commented Jul 18, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 18, 2023
@dcibot
Copy link

dcibot commented Jul 18, 2023

bcrochet
bcrochet requested changes Jul 19, 2023
View reviewed changes
Copy link
Contributor

@bcrochet bcrochet left a comment

Choose a reason for hiding this comment

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

Couple of changes.

internal/policy/container/has_modified_files.go Outdated Show resolved Hide resolved
internal/policy/container/has_modified_files.go Outdated Show resolved Hide resolved
internal/policy/container/has_modified_files.go Outdated Show resolved Hide resolved
@acornett21 acornett21 force-pushed the fixes_has_modified_files_cross_arch_support branch from 5449729 to 689c725 Compare July 19, 2023 18:31
@dcibot
Copy link

dcibot commented Jul 19, 2023

@acornett21 acornett21 force-pushed the fixes_has_modified_files_cross_arch_support branch 2 times, most recently from 4062e33 to caa3eff Compare July 19, 2023 20:04
@dcibot
Copy link

dcibot commented Jul 19, 2023

komish
komish reviewed Jul 19, 2023
View reviewed changes
internal/policy/container/has_modified_files.go Outdated
@@ -241,7 +261,9 @@ func (p *HasModifiedFilesCheck) validate(ctx context.Context, layerIDs []string,

if (previousOsRelease && !currentOsRelease) || (previousPackage.Arch != currentPackage.Arch) {
// If either of these differ, that's a fail
return false, nil
logger.V(log.DBG).Info("mismatch in package architecture", "file", modifiedFile)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this phrasing correct? This log line seems to imply only the arch could be wrong here. Should we ever hit this case for x86_64 || i686?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will not hit this case coded for the above code. This was the code block that was originally causing a failure and were never logging this and it caused confusion for cert-ops. I'd like to make this line better, but I need to re-read @bcrochet comments about what this if block covers and comment accordingly, that should yield a better log message.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make them more explicit. It makes sense to split these two so that we get a more granular message.
This is still valid. This is the case that someone replaced a Red Hat package with one of a different arch. OR, they did a rebuild, and the release changed.

EIther way, it shouldn't pass. But making it separate will make the code more readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I split this into two different if conditions and added a unit test to cover both cases.

komish
komish approved these changes Jul 19, 2023
View reviewed changes
Copy link
Contributor

@komish komish left a comment

Choose a reason for hiding this comment

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

Seems fine

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 19, 2023
@acornett21 acornett21 force-pushed the fixes_has_modified_files_cross_arch_support branch from caa3eff to 84b4135 Compare July 19, 2023 21:01
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 19, 2023
@dcibot
Copy link

dcibot commented Jul 19, 2023

@acornett21 acornett21 force-pushed the fixes_has_modified_files_cross_arch_support branch from 84b4135 to e673ecf Compare July 20, 2023 17:23
@dcibot
Copy link

dcibot commented Jul 20, 2023

@acornett21 acornett21 force-pushed the fixes_has_modified_files_cross_arch_support branch 4 times, most recently from 582e8b5 to c71ca4e Compare July 24, 2023 19:57
@dcibot
Copy link

dcibot commented Jul 24, 2023

@acornett21 acornett21 force-pushed the fixes_has_modified_files_cross_arch_support branch from c71ca4e to b728d49 Compare July 26, 2023 19:02
@dcibot
Copy link

dcibot commented Jul 26, 2023

@acornett21 acornett21 force-pushed the fixes_has_modified_files_cross_arch_support branch from b728d49 to 4b8991d Compare July 26, 2023 21:50
@dcibot
Copy link

dcibot commented Jul 26, 2023

@acornett21 acornett21 force-pushed the fixes_has_modified_files_cross_arch_support branch from 4b8991d to 552fb83 Compare July 27, 2023 15:37
@dcibot
Copy link

dcibot commented Jul 27, 2023

@acornett21
adding rpm architecture of to our maps, so 32 bit and 64 bit rpms do …
ea0aae1 
…not fail has modified files check

Signed-off-by: Adam D. Cornett <adc@redhat.com>
@acornett21 acornett21 force-pushed the fixes_has_modified_files_cross_arch_support branch from 552fb83 to ea0aae1 Compare July 27, 2023 16:52
bcrochet
bcrochet approved these changes Jul 27, 2023
View reviewed changes
Copy link
Contributor

@bcrochet bcrochet left a comment

Choose a reason for hiding this comment

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

/lgtm

@dcibot
Copy link

dcibot commented Jul 27, 2023

komish
komish approved these changes Jul 27, 2023
View reviewed changes
Copy link
Contributor

@komish komish left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 27, 2023
@openshift-ci
Copy link

openshift-ci bot commented Jul 27, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: acornett21, bcrochet, komish

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:
  • OWNERS [acornett21,bcrochet,komish]

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

@acornett21 acornett21 marked this pull request as ready for review July 27, 2023 19:27
@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 Jul 27, 2023
@openshift-ci openshift-ci bot requested review from bcrochet and jomkz July 27, 2023 19:27
@acornett21 acornett21 merged commit 9234b2e into redhat-openshift-ecosystem:main Jul 27, 2023
5 checks passed
@acornett21 acornett21 deleted the fixes_has_modified_files_cross_arch_support branch January 2, 2024 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@komish komish komish approved these changes

@bcrochet bcrochet Awaiting requested review from bcrochet

@jomkz jomkz Awaiting requested review from jomkz

Assignees

@bcrochet bcrochet

@komish komish

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
4 participants
@acornett21 @dcibot @bcrochet @komish