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

require conmon v2.0.1 #3792

Merged
merged 4 commits into from
Oct 29, 2019

Conversation

haircommander
Copy link
Collaborator

@haircommander haircommander commented Aug 12, 2019

Signed-off-by: Peter Hunt [email protected]

@mheon
Copy link
Member

mheon commented Aug 12, 2019

LGTM

@TomSweeneyRedHat
Copy link
Member

Changes LGTM, but some tests aren't hip

// minConmonMajor is the major version required for conmon
minConmonMajor = 2
// minConmonVersion is a string used to display the minimum conmon version
minConmonVersion = "v2.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

Can you make this
minConmonVersion = fmt.Sprintf("v%d.0.0", minConmonMajor)

Copy link
Member

Choose a reason for hiding this comment

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

Or better yet remove this variable and just use the minConmonMajor in the error message below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@@ -829,7 +829,7 @@ func makeRuntime(ctx context.Context, runtime *Runtime) (err error) {

if !foundConmon {
if foundOutdatedConmon {
return errors.Wrapf(define.ErrConmonOutdated, "please update to v1.0.0 or later")
return errors.Wrapf(define.ErrConmonOutdated, "please update to v%s.0.0 or later", minConmonMajor)
Copy link
Member

Choose a reason for hiding this comment

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

Should be v%d.0.0

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oopsies

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

golang is so forgiving...

@rhatdan
Copy link
Member

rhatdan commented Aug 12, 2019

/lgtm
/hold
Feel free to hold cancel when tests pass.

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Aug 12, 2019
@mheon
Copy link
Member

mheon commented Aug 15, 2019

Large quantities of test failures - does this need a rebase?

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 15, 2019
@haircommander
Copy link
Collaborator Author

@mheon let's try that :D

@mheon
Copy link
Member

mheon commented Aug 22, 2019

/approve

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 22, 2019
@mheon
Copy link
Member

mheon commented Sep 12, 2019

@haircommander Any chance we can get a rebase for the 1.6.0 release?

@haircommander
Copy link
Collaborator Author

@mheon sure, should we do 2.0.1 (the eventual new conmon)?

I'll need @lsm5 to integrate conmon into fedora before this could merge

@mheon
Copy link
Member

mheon commented Sep 12, 2019

If we can get 2.0.1 images, that'd be good - otherwise, ensuring 2.0.0 is probably sufficient.

@lsm5
Copy link
Member

lsm5 commented Sep 12, 2019

@mheon sure, should we do 2.0.1 (the eventual new conmon)?

I'll need @lsm5 to integrate conmon into fedora before this could merge

done on rawhide already. I'll get it on the rest asap.

@rhatdan
Copy link
Member

rhatdan commented Sep 13, 2019

Conmon is now in place on F30 and f31.

@mheon
Copy link
Member

mheon commented Oct 28, 2019

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 28, 2019
@mheon
Copy link
Member

mheon commented Oct 28, 2019

Hung on a failed test

@mheon
Copy link
Member

mheon commented Oct 28, 2019

Restarted podman-in-podman but failures might be legitimate

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 28, 2019
@haircommander
Copy link
Collaborator Author

haircommander commented Oct 28, 2019

sorry folks, I think I missed the step of updating the dockerfile. I think in_podman should pass now?

@rhatdan
Copy link
Member

rhatdan commented Oct 28, 2019

/lgtm
/hold
hold cancel when tests pass.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 28, 2019
@haircommander
Copy link
Collaborator Author

@cevich am I correct in saying that this PR needs to be merged before the changes in the dockerfile get used in the in_podman task? If that were the case, this would need to be force merged when all other tests pass

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 28, 2019
@haircommander
Copy link
Collaborator Author

manually pushed to the in_podman image. It should pass, then once we merge the post merge action will overwrite it organically. can I have a new lgtm @rhatdan @baude @vrothberg @TomSweeneyRedHat @mheon

@haircommander
Copy link
Collaborator Author

happy tests PTAL

@@ -57,7 +57,7 @@ RUN set -x \
&& rm -rf "$GOPATH"

# Install conmon
ENV CONMON_COMMIT 6f3572558b97bc60dd8f8c7f0807748e6ce2c440
ENV CONMON_COMMIT 65fe0226d85b69fc9e527e376795c9791199153d
Copy link
Member

Choose a reason for hiding this comment

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

This is fine for now, but consider changing this over to the tag for the release like 'v2.0.2' then below I think (didn't try it) you can do git checkout tags/CONMON_COMMIT, at least that's the theory.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think actually the next step is to enable pulling conmon from a package (like the other vms do) instead of by git commit

@TomSweeneyRedHat
Copy link
Member

LGTM

@rhatdan
Copy link
Member

rhatdan commented Oct 29, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 29, 2019
@vrothberg
Copy link
Member

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 29, 2019
@vrothberg
Copy link
Member

Note that the code will slightly change as soon as I need to merge it into #4352

@openshift-merge-robot openshift-merge-robot merged commit 59582c5 into containers:master Oct 29, 2019
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 26, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants