Skip to content

Conversation

@cevich
Copy link
Member

@cevich cevich commented Jul 12, 2021

Taking over #1350

@cevich cevich changed the title Implement manpage checker in CI [CI:DOCS] Implement manpage checker in CI Jul 12, 2021
@cevich cevich force-pushed the man_page_checker_3 branch 2 times, most recently from 0048b26 to 8143d10 Compare July 12, 2021 15:40
**--cert-dir** _path_ Use certificates at _path_ (*.crt, *.cert, *.key) to connect to the registry.
**--help**, **-h**

**--tls-verify** _bool-value_ Require HTTPS and verify certificates when talking to container registries (defaults to true).
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to make sure this isn’t lost, there’s been a fairly long discussion / research around the --tls-verify flag, #1350 (comment) .

Copy link
Member Author

Choose a reason for hiding this comment

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

Oof, yeah, "long" is right. Would you mind opening an issue about it? I feel that might be a better way to ensure the discussion isn't lost, since addressing it in this (or that #1350) PR seems onerous.

Copy link
Contributor

Choose a reason for hiding this comment

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

The thing is, this PR is mostly blocked on that discussion — without fixing --help, enforcing correspondence between --help and man pages would make the state of documentation worse.

But contrary to the bleak mood of most of that discussion, there might be a fairly clean way forward now: #1369 . It definitely needs an independent review, but looks promising.

@cevich cevich force-pushed the man_page_checker_3 branch 3 times, most recently from ebcda72 to 4cd39c1 Compare July 12, 2021 16:02
@cevich cevich changed the title [CI:DOCS] Implement manpage checker in CI Implement manpage checker in CI Jul 12, 2021
@cevich cevich force-pushed the man_page_checker_3 branch from 4cd39c1 to f115516 Compare July 12, 2021 16:27
@cevich cevich changed the title Implement manpage checker in CI [WIP] Implement manpage checker in CI Jul 12, 2021
@cevich
Copy link
Member Author

cevich commented Jul 12, 2021

DO NOT MERGE: Holding for #1375

@cevich
Copy link
Member Author

cevich commented Jul 12, 2021

Stuck on an osx failure:

export PATH=$GOPATH/bin:$PATH
brew update
remote: fatal: packfile /data/repositories/b/nw/b6/07/5c/123272362/network.git/objects/pack/pack-b33165f7e4627d75eb4db0f1f037c9cfe8722882.pack cannot be accessed        
fatal: protocol error: bad pack header
Error: Fetching /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core failed!
...cut...
You have 1 outdated formula and 2 outdated casks installed.
You can upgrade them with brew upgrade
or list them with brew outdated.

I'm hoping that's just some kind of flake (I already re-ran it once). Is "update" and "upgrade" the same thing?

@mtrmac
Copy link
Contributor

mtrmac commented Jul 12, 2021

brew update = upgrade Homebrew itself, download fresh metadata.

brew upgrade = given current metadata, upgrade any currently installed packages (but not Homebrew itself, which uses the above special mechanism)

@cevich
Copy link
Member Author

cevich commented Jul 12, 2021

Hmmm, so it sounds like we should do an update then and upgrade? Though I'm also concerned about the remote: fatal: packfile error, and wondering why isn't a non-zero exit thrown? Maybe just a poorly written error message?

@mtrmac
Copy link
Contributor

mtrmac commented Jul 12, 2021

Hmmm, so it sounds like we should do an update then and upgrade

I think ideally we should do neither: the images we deploy should be frequently refreshed with the latest Homebrew, and we would just do brew install. At least historically with Travis IIRC we’ve changed that with a good (and hopefully documented?) reason, I can’t recall looking into the Cirrus situation.

Though I'm also concerned about the remote: fatal: packfile error, and wondering why isn't a non-zero exit thrown? Maybe just a poorly written error message?

No idea. As a very wild guess this just looks like an infrastructure issue, either at the Homebrew’s sponsor that provides hosting for downloads, or within the current Cirrus image, and especially if it’s the latter it might be one that isn’t encountered frequently enough to be handled quite correctly. Checking reports in https://github.com/homebrew/ might be informative, but it’s too late for me today.

@cevich
Copy link
Member Author

cevich commented Jul 13, 2021

the images we deploy should be frequently refreshed with the latest Homebrew

For Mac, this is somewhat out of our control as the images are managed by Cirrus (I believe they're rebuilt/refreshed at some interval). Same story for Windows. The only viable alternatives for automated Mac testing involve either less control and added code duplication (github actions) or TONs more scripting and plumbing work (Cirrus + paid MacStadium account). Basically...it would be better for humanity if Apple behaved more like Linux 😁

At least historically with Travis IIRC we’ve changed that with a good (and hopefully documented?) reason, I can’t recall looking into the Cirrus situation.

Travis routinely was taking an hour to start running a 5-minute job, as well as severely limiting the total minutes-quota available for FOSS projects to use. So it's not really a choice to leave for something else, we really needed to drop Travis to avoid severe impacts to development.

No idea. As a very wild guess this just looks like an infrastructure issue

This was the case, I re-ran the job today and it passed. Still, it's concerning that (the tool) doesn't throw a non-zero exit on a fatal error. It's possible, thought I highly doubt Cirrus-CI is not setting +e for the shell (they're pretty careful about that stuff elsewhere).

@cevich cevich force-pushed the man_page_checker_3 branch from f115516 to 16232b7 Compare July 13, 2021 13:59
@cevich
Copy link
Member Author

cevich commented Jul 13, 2021

(rebase)

@mtrmac
Copy link
Contributor

mtrmac commented Jul 13, 2021

I was absolutely not suggesting this as a reason to return to Travis; I just wanted to point at the

skopeo/.travis.yml

Lines 62 to 66 in 146af8c

# Ideally, the (brew update) should not be necessary and Travis would have fairly
# frequently updated OS images; that's not been the case historically.
# In particular, explicitly unlink python@2, which has been removed from Homebrew
# since the last OS image build (as of July 2020), but the Travis OS still
# contains it, and it prevents updating of Python 3.
comment block documenting why+what.

It’s was not immediately clear to me (OTOH I hadn’t looked into the history at all deeply when writing my previous comment) whether the Cirrus version was doing brew update for a reason … and it turns out there was a reason, documented in the commit message of #1329 , and I have nobody to blame but myself for this goose chase.

@cevich
Copy link
Member Author

cevich commented Jul 13, 2021

I have nobody to blame but myself for this goose chase.

No worries, at least we can rely on git/github to keep a record of stuff of our failing memories 😁

@cevich cevich changed the title [WIP] Implement manpage checker in CI Implement manpage checker in CI Jul 21, 2021
This is the script that runs 'skopeo COMMAND --help' and
cross-checks that all the option flags are documented
in man pages, and vice-versa (all options listed in man
pages appear in COMMAND's --help message).

Copied from podman, with changes for skopeo-land (removing
the rst checks, and conforming to skopeo conventions).

Signed-off-by: Ed Santiago <santiago@redhat.com>
@cevich cevich force-pushed the man_page_checker_3 branch from 16232b7 to e2aefcf Compare July 21, 2021 18:51
@cevich
Copy link
Member Author

cevich commented Jul 21, 2021

I believe this is ready to go. @edsantiago and @mtrmac PTAL.

@cevich
Copy link
Member Author

cevich commented Jul 21, 2021

note-to-self: Need to copy-paste the .cirrus.yml changes to to the test_skopeo task over to c/image.

Copy link
Contributor

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

The CI automation part looks plausible to me, but I only very minimally understand it and I’ll happily defer to experts.

What about the --tls-verify part, #1374 (comment) ?

@edsantiago Could you see whether there are any obvious holes in #1369 that I might have missed, please? You probably understand the gotchas of Cobra better than I do.

(If that plan works, I’d be perfectly fine with merging this PR first, and taking on the work to actually update man pages in the other PR myself.)

Signed-off-by: Chris Evich <cevich@redhat.com>
@cevich cevich force-pushed the man_page_checker_3 branch from e2aefcf to 02bacf5 Compare July 22, 2021 14:10
@cevich
Copy link
Member Author

cevich commented Jul 22, 2021

but I only very minimally understand it

My overarching goal is for it to be at least somewhat approachable for everyone. I'm not always around to fix/maintain things and am deliberately attempting to not be a bottleneck. I realized that automation is somewhat of a niche specialization, but if I can help make it any clearer with comments or different names or links to docs or anything, please let me know.

@edsantiago
Copy link
Member

edsantiago commented Jul 22, 2021

/lgtm

@mtrmac can you rebase #1369 on this, adding back the removed --tls-verify to manpages? (After it merges, that is: looks like I don't have /lgtm privs in this repo)

@cevich
Copy link
Member Author

cevich commented Jul 22, 2021

looks like I don't have /lgtm privs in this repo

There's no bot setup here.

@rhatdan rhatdan merged commit 3c2d988 into containers:main Jul 22, 2021
mtrmac added a commit to mtrmac/image that referenced this pull request Jul 23, 2021
It was removed in containers/skopeo#1374 .

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Copy link
Contributor

mtrmac commented Jul 23, 2021

but I only very minimally understand it

My overarching goal is for it to be at least somewhat approachable for everyone. I'm not always around to fix/maintain things and am deliberately attempting to not be a bottleneck. I realized that automation is somewhat of a niche specialization, but if I can help make it any clearer with comments or different names or links to docs or anything, please let me know.

No worries; I feel reasonably confident in making targeted small changes, it’s primarily an “I don’t know what I don’t know” worry.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants