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

[CI:DOCS] Man pages: refactor common options: --ipc #15453

Merged
merged 1 commit into from
Aug 24, 2022

Conversation

edsantiago
Copy link
Member

This is not an easy one to review, sorry.

I went with the version from podman-create. The differences
against podman-run are subtle: apostrophes, whitespace, and
the arg description in the '####' line. Suggestion for review:
run hack/markdown-preprocess-review, then after you finish
with that, cd /tmp/markdown/ipc and use your favorite
two-file diff tool to compare podman-run* against zzz*.

I did not even try to combine the podman-build one; that one
is too different.

Signed-off-by: Ed Santiago [email protected]

more man-page deduplication

This is not an easy one to review, sorry.

I went with the version from podman-create. The differences
against podman-run are subtle: apostrophes, whitespace, and
the arg description in the '####' line. Suggestion for review:
run hack/markdown-preprocess-review, then after you finish
with that, cd /tmp/markdown<TAB>/ipc and use your favorite
two-file diff tool to compare podman-run* against zzz*.

I did not even try to combine the podman-build one; that one
is too different.

Signed-off-by: Ed Santiago <[email protected]>
@openshift-ci openshift-ci bot added release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 24, 2022
@edsantiago
Copy link
Member Author

The podman-build version, for comparison:

#### **--ipc**=*how*
Sets the configuration for IPC namespaces when handling `RUN` instructions.
The configured value can be "" (the empty string) or "container" to indicate
that a new IPC namespace should be created, or it can be "host" to indicate
that the IPC namespace in which `podman` itself is being run should be reused,
or it can be the path to an IPC namespace which is already in use by
another process.

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

@rhatdan
Copy link
Member

rhatdan commented Aug 24, 2022

/approve
/lgtm
/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 24, 2022
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 24, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 24, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, 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:
  • OWNERS [edsantiago,rhatdan,vrothberg]

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

@edsantiago
Copy link
Member Author

EPIPELINEEMPTY. I had a stash of these queued up for submitting one by one, but this is the last one, and I'm too busy on other things to work on the next ones! #firstworldproblems

@mheon
Copy link
Member

mheon commented Aug 24, 2022

What are these new production-build jobs from? Don't recall seeing any CI work on this.

@rhatdan
Copy link
Member

rhatdan commented Aug 24, 2022

@lsm5 @cevich PTAL

@edsantiago
Copy link
Member Author

That looks like the new packit thing (#15375). It merged before I got to review it. @lsm5 PTAL.

@lsm5
Copy link
Member

lsm5 commented Aug 24, 2022

error: Installed (but unpackaged) file(s) found:
   /usr/share/user-tmpfiles.d/podman-docker.conf

Did we miss this earlier ?

@edsantiago
Copy link
Member Author

Merge collision, #15444

@lsm5
Copy link
Member

lsm5 commented Aug 24, 2022

bumped dist-git spec files to account for /usr/lib/tmpfiles.d/podman-docker.conf.

@lsm5
Copy link
Member

lsm5 commented Aug 24, 2022

@cevich looks like packit has much better status reporting in github UI compared to other tests.

@edsantiago
Copy link
Member Author

So, that's what took me so long to review yesterday, and why I didn't finish. I was trying to figure out all the implications of cross-depending github and dist-git. I still don't think I understand it all, but ISTM that we can get into situations in which CI is deadlocked waiting for you to update dist-git; and then if you do update dist-git, other in-flight PRs will fail? I need to draw a full diagram.

@lsm5
Copy link
Member

lsm5 commented Aug 24, 2022

What are these new production-build jobs from? Don't recall seeing any CI work on this.

@mheon there's a new .packit.yaml which will run scratch koji builds using fedora's dist-git sources patched to build the PR commit. Good to check for build issues on all arches and fix them prior to merge. Packit will also allow running tests, but that seemed to be a bit more involved, so I'm gonna explore that in stages.

@lsm5
Copy link
Member

lsm5 commented Aug 24, 2022

So, that's what took me so long to review yesterday, and why I didn't finish. I was trying to figure out all the implications of cross-depending github and dist-git. I still don't think I understand it all, but ISTM that we can get into situations in which CI is deadlocked waiting for you to update dist-git; and then if you do update dist-git, other in-flight PRs will fail? I need to draw a full diagram.

AFAICT, the current packit jobs will fail if:

  • there are new files installed. In this case it should be trivial to update the file list to accommodate new files. If the new files can't be added to dist-git right away, then I'd say we can/should ignore packit failures.

  • there are actual build issues. In this case, this would be an early warning for us, which I see as a good thing.

@lsm5
Copy link
Member

lsm5 commented Aug 24, 2022

ugh, didn't notice the new file path

@edsantiago
Copy link
Member Author

looks like packit has much better status reporting in github UI compared to other tests.

You mean the thing where it goes into spinning-yellow-circle when the test restarts, instead of red-X? Yeah, that REALLY irks me about Cirrus! And packit shows that it's possible to do it correctly!

@lsm5
Copy link
Member

lsm5 commented Aug 24, 2022

Given the new file is /usr/lib/*user-*tmpfiles.d/podman-docker.conf, it can't be included in dist-git AFAICT, unless I do some unholy wildcarding. So, let's ignore packit failures. I don't think it should block from auto-merging.

@lsm5
Copy link
Member

lsm5 commented Aug 24, 2022

I'll check with packit people if they have any alternatives for this issue.

@edsantiago
Copy link
Member Author

Given the new file is /usr/lib/*user-*tmpfiles.d/podman-docker.conf, it can't be included in dist-git AFAICT

Meaning, #15444 violates FHS? If so, can you submit a podman PR to fix it, or tell me how to do so?

@edsantiago edsantiago removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 24, 2022
@lsm5
Copy link
Member

lsm5 commented Aug 24, 2022

Given the new file is /usr/lib/*user-*tmpfiles.d/podman-docker.conf, it can't be included in dist-git AFAICT

Meaning, #15444 violates FHS? If so, can you submit a podman PR to fix it, or tell me how to do so?

Nope, my bad. I do see systemd mention that path. I fixed spec file in the wrong place. Let me make a quick change and give it 1 last try.

@openshift-merge-robot openshift-merge-robot merged commit 34d5168 into containers:main Aug 24, 2022
@edsantiago edsantiago deleted the docs_dedup_ipc branch August 24, 2022 14:15
@edsantiago
Copy link
Member Author

This has merged, but I'm quite sure the problem is affecting all other in-flight PRs.

@lsm5
Copy link
Member

lsm5 commented Aug 24, 2022

This has merged, but I'm quite sure the problem is affecting all other in-flight PRs.

I pushed another spec file update to account for that path, correctly :D . Can you rerun the failures you see please?

@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 20, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 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. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants