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

Simplify PullPolicy handling #1922

Merged
merged 1 commit into from
Mar 22, 2024
Merged

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Mar 20, 2024

Add support for ifmissing

pkg/config/config_test.go Show resolved Hide resolved
policy: PullPolicyNewer,
},
{
value: "NEVER",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
value: "NEVER",
value: "NEWER",

Also needs IFNEWER,

Copy link
Member Author

Choose a reason for hiding this comment

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

These are fixed in current PR.

pkg/config/config_test.go Show resolved Hide resolved
@rhatdan
Copy link
Member Author

rhatdan commented Mar 20, 2024

@mheon @edsantiago @Luap99 @cevich Any idea what is blowing up here?

p, err := ParsePullPolicy(test.value)
if test.fail {
gomega.Expect(err.Error()).To(gomega.Equal(fmt.Sprintf("unsupported pull policy %q", test.value)))

Copy link
Collaborator

Choose a reason for hiding this comment

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

gofumpt is barfing about this blank line

@edsantiago
Copy link
Collaborator

Followup for future reference. The error is:

  Error: File is not `gofumpt`-ed (gofumpt)

gofumpt is:

# dnf search gofumpt
Last metadata expiration check: 1:12:43 ago on Wed 20 Mar 2024 08:41:20 AM MDT.
============================ Name Matched: gofumpt =============================
golang-mvdan-gofumpt.x86_64 : A stricter gofmt

Easy enough to install and run

@rhatdan
Copy link
Member Author

rhatdan commented Mar 20, 2024

So there is something wrong with the VM image? Or is there something wrong with my PR?

@edsantiago
Copy link
Collaborator

See my inline comment in your diff. I believe this strict linter is catching something that gofmt did not, and IMO it's a good catch.

Add support for ifmissing

Signed-off-by: Daniel J Walsh <[email protected]>
@rhatdan
Copy link
Member Author

rhatdan commented Mar 21, 2024

The way the tool indicated this problem was to throw up in the logs about missing packages, which is very confusing.

@rhatdan
Copy link
Member Author

rhatdan commented Mar 21, 2024

@edsantiago @vrothberg @Luap99 PTAL

@mheon
Copy link
Member

mheon commented Mar 21, 2024

One more comment from @vrothberg is still relevant. Once that's addressed LGTM

@rhatdan
Copy link
Member Author

rhatdan commented Mar 22, 2024

@vrothberg PTAL I think I Have addressed all your issues.

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link
Contributor

openshift-ci bot commented Mar 22, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan, vrothberg

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

@vrothberg
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Mar 22, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 06eb2ea into containers:main Mar 22, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants