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

search test on fedora registry: retry 5 times #5284

Merged

Conversation

edsantiago
Copy link
Member

...to try to compensate for flaky host.

registry.fedoraproject.org is just not reliable. It's flaking
with 503 errors, causing massive amounts of wasted CI time
and developer effort.

There is exactly one instance of that registry in these tests.
We can't replace it with quay.io, because "search quay.io/"
(trailing slash) fails with some sort of authentication error.
So let's just try a sleep/retry cycle instead.

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

@@ -165,8 +166,16 @@ registries = ['{{.Host}}:{{.Port}}']`
})

It("podman search v2 registry with empty query", func() {
search := podmanTest.Podman([]string{"search", "registry.fedoraproject.org/"})
search.WaitWithDefaultTimeout()
var search
Copy link
Member

Choose a reason for hiding this comment

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

Compiler is yelling because it's not sure what type this is... I'm honestly not either. You might need to make this search := podmanTest.Podman(...) so the compiler infers the type

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm honestly not either.

Oh, thank you. I've been pulling my hair out trying to figure out the type (I can't get ginkgo working on my laptop, so I can't experiment either).

If I use := won't it fall out of scope outside the for loop? /me doesn't grok golang.

Copy link
Member

Choose a reason for hiding this comment

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

var search *PodmanSessionIntegration

Copy link
Member

Choose a reason for hiding this comment

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

should work

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, := is just sugar for var $NAME <inferred type> = ...

Copy link
Member Author

Choose a reason for hiding this comment

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

@mheon, I know that much^Wlittle about Go; my question was, doesn't := declare within curlybrace scope? If I did session := ..., could I use session in the two Expect()s below?

Copy link
Member

Choose a reason for hiding this comment

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

Yes - it'll leak into the sub-scope created by the curly braces (though you can declare another session within that sub-scope to shadow it in there).

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm still not using the right words. Let me try this way. Will this work? I wouldn't expect it to.

    for .... {
        myvar := autodeclared
        ....
    }
    if myvar ...     // <<---- referencing myvar outside of the above "for" scope

Copy link
Member Author

Choose a reason for hiding this comment

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

PS thank you @rhatdan, that seems to be getting it much further along.

...to try to compensate for flaky host.

registry.fedoraproject.org is just not reliable. It's flaking
with 503 errors, causing massive amounts of wasted CI time
and developer effort.

There is exactly one instance of that registry in these tests.
We can't replace it with quay.io, because "search quay.io/"
(trailing slash) fails with some sort of authentication error.
So let's just try a sleep/retry cycle instead.

Signed-off-by: Ed Santiago <[email protected]>
@edsantiago edsantiago force-pushed the flaky_fedora_registry branch from d6d56e4 to bb31d35 Compare February 20, 2020 20:13
@TomSweeneyRedHat
Copy link
Member

LGTM

@edsantiago
Copy link
Member Author

Neither test (fedora-30 nor ubuntu-18) experienced an error, i.e. both succeeded on the first attempt to fedoraregistry, so the retry code never triggered.

@rhatdan
Copy link
Member

rhatdan commented Feb 20, 2020

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 20, 2020
@jwhonce jwhonce added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 20, 2020
@mheon
Copy link
Member

mheon commented Feb 20, 2020

/approve
/lgtm

Not sure what's going on here...

@edsantiago
Copy link
Member Author

githubstatus.com is reporting a recently (minutes ago) resolved issue

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, mheon, 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-merge-robot openshift-merge-robot merged commit a8896d5 into containers:master Feb 20, 2020
@edsantiago edsantiago deleted the flaky_fedora_registry branch February 20, 2020 22:57
@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 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 25, 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.

7 participants