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

Cirrus: Add packages that provide htpasswd #6822

Merged
merged 3 commits into from
Jul 22, 2020

Conversation

cevich
Copy link
Member

@cevich cevich commented Jun 30, 2020

Mainly needed for buildah testing: the htpasswd command was removed from
the upstream registry container image. Making it available on the
host-side enables configuring details needed by the registry during
it's initial setup.

Signed-off-by: Chris Evich [email protected]

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 30, 2020
@cevich cevich force-pushed the add_htpasswd branch 4 times, most recently from 30c0b51 to 35d3631 Compare July 2, 2020 17:33
@cevich cevich force-pushed the add_htpasswd branch 6 times, most recently from cc45823 to 1653581 Compare July 8, 2020 20:10
@edsantiago
Copy link
Member

Oh good, you're getting the same varlink failures I saw. I'll be curious to see how you resolve those.

@cevich cevich added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jul 8, 2020
@cevich cevich changed the title WIP: CI:IMG: Cirrus: Add packages that provide htpasswd WIP: Cirrus: Add packages that provide htpasswd Jul 8, 2020
@cevich cevich changed the title WIP: Cirrus: Add packages that provide htpasswd Cirrus: Add packages that provide htpasswd Jul 8, 2020
@edsantiago
Copy link
Member

As best I can interpret, you worked around the varlink problem by just sweeping it under the rug? Doesn't that defer the problem for the next person who does a CI:IMG PR? Would it make sense to at least file an issue? Or have I missed something?

@cevich
Copy link
Member Author

cevich commented Jul 9, 2020

Oh good, you're getting the same varlink failures I saw. I'll be curious to see how you resolve those.

I was half-listening to the chatter about varlink problems during scrum, but I did nothing to resolve other than rebase 😁

Or have I missed something?

Hmm, not sure, or maybe I'm missing something?

TBH I didn't even notice any varlink problems...AFAIR, the main issue I ran into here was a combination of:

  • Treating $GOPATH and $GOSRC between testing runtime and VM image build time, differently (value-wise).
  • After fixing that, so everything was same-same, I hit a problem building ginkgo/gomega. Brent helped me look at it, and we found (surprise!) it was related to $GOPATH.

I ended up needing to hop onto a VM (with git_ci_vm.sh) to locate the issue in my scripts. Once that was fixed, and (IIRC another rebase), I got an image built successfully and things "just worked" after that.

Does that help? Is there something I need to look into more so we can be sure I didn't rug-sweep on accident?

@edsantiago
Copy link
Member

CI failed last night (log) with:

[+0003s] not ok The varlink executable is present: -x 

...which is exactly what I saw in my #6869 (see there for further details on what's failing; it caused me to give up on my PR). Just a few seconds after that failure, you changed the title (removing CI:IMG) and force-pushed a new commit that updated BUILT_IMAGE_SUFFIX but with no change to the failing varlink test. I was just trying to understand why.

If these VMs are only going to be used on the main development branch, it's probably OK to remove the varlink test, and that was going to be my approach. But if these VMs are used on any older branches (which may still need varlink), things may fail in surprising ways. I don't know enough about the varlink situation to understand the ramifications.

@mheon
Copy link
Member

mheon commented Jul 9, 2020

Should we just drop that check entirely? We have a completely separate varlink_api test in Cirrus that should, among other things, verify we have a working varlink executable?

@cevich
Copy link
Member Author

cevich commented Jul 9, 2020

but with no change to the failing varlink test. I was just trying to understand why..

Okay sure, yeah I have no doubts as to your motivation, and I think I understand your confusion as well. I'm pretty rigorous about rebasing my PRs when they're close to being "done". That's all I got in terms of "likely explanation" of why it suddenly started working. Too many PRs, too many cycles, too tired old brain 😕

As for your other question/suggestion, and...

Should we just drop that check entirely? We have a completely separate varlink_api test in Cirrus that should, among other things, verify we have a working varlink executable?

I think we should. We have other branches with their own relevant flavor of the CI setup that can be relied upon to test things as required. Since (I'm assuming) varlink isn't needed on the main branch, let's kill it, and have one less thing to possibly flake or do strange things that make Ed slam the door on his own PRs in frustration 😁

(Not to make fun...I've been in your situation Ed, the frustration is real. TBH, there are some parts of CI I don't understand how they work fully. This is where having/trusting a great team is sooo very important.)

@rhatdan
Copy link
Member

rhatdan commented Jul 22, 2020

@cevich what should we do with this PR?

cevich added 2 commits July 22, 2020 11:51
Mainly needed for buildah testing: the htpasswd command was removed from
the upstream registry container image.  Making it available on the
host-side enables configuring details needed by the registry during
it's initial setup.

Signed-off-by: Chris Evich <[email protected]>
Note: The libpod -> podman change in the image name comes by way of an
intentional repository rename.

Signed-off-by: Chris Evich <[email protected]>
@cevich
Copy link
Member Author

cevich commented Jul 22, 2020

@rhatdan I'm going to rebase once more, and then I think this is good to merge.

@edsantiago
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 22, 2020
@rhatdan
Copy link
Member

rhatdan commented Jul 22, 2020

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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

@cevich could you remove the WIP label so we can get this merged? The new images contain crun 0.14, which should (FINALLY) fix the cgroup.freeze flakes that are still affecting us.

@cevich
Copy link
Member Author

cevich commented Jul 22, 2020

Huh? Oh I bet that got stuck because of the rename...

@cevich cevich removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 22, 2020
@openshift-merge-robot openshift-merge-robot merged commit 0da4fa2 into containers:master Jul 22, 2020
edsantiago added a commit to edsantiago/libpod that referenced this pull request Jul 23, 2020
CI runs are failing in special_testing_rootless:

    mkdir /var/tmp/go/pkg: permission denied

Probable cause: containers#6822, which universally set GOPATH.

Solution: in rootless setup, chown -R GOPATH as well
as GOSRC (the latter was already being chowned).

Signed-off-by: Ed Santiago <[email protected]>
@cevich cevich deleted the add_htpasswd branch June 30, 2021 18:06
@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 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 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.

6 participants