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

Bug 2006947: fix proxy portion of tbr inaccessible check #397

Merged
merged 1 commit into from Sep 27, 2021

Conversation

gabemontero
Copy link
Contributor

No description provided.

@openshift-ci openshift-ci bot added bugzilla/severity-unspecified Referenced Bugzilla bug's severity is unspecified for the PR. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Sep 22, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 22, 2021

@gabemontero: This pull request references Bugzilla bug 2006947, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.10.0) matches configured target release for branch (4.10.0)
  • bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @jitendar-singh

In response to this:

Bug 2006947: fix proxy portion of tbr inaccessible check

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@gabemontero
Copy link
Contributor Author

/bugzilla refresh

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 22, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 22, 2021

@gabemontero: This pull request references Bugzilla bug 2006947, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.10.0) matches configured target release for branch (4.10.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @jitendar-singh

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@gabemontero
Copy link
Contributor Author

/bugzilla refresh

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 22, 2021

@gabemontero: This pull request references Bugzilla bug 2006947, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.10.0) matches configured target release for branch (4.10.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @jitendar-singh

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci openshift-ci bot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. and removed bugzilla/severity-unspecified Referenced Bugzilla bug's severity is unspecified for the PR. labels Sep 22, 2021
@gabemontero
Copy link
Contributor Author

/hold

until e2e-aws-proxy is available

@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 Sep 23, 2021
@gabemontero
Copy link
Contributor Author

hard to tell ... probably a mysql flake in image eco give it passed earlier today and I see no recent commits there or in cakephp

non related flakes in okd

/skip

@gabemontero
Copy link
Contributor Author

/retest

@gabemontero
Copy link
Contributor Author

and we have a e2e-aws-proxy up and running

@gabemontero
Copy link
Contributor Author

aws fail on image ecosystem

@gabemontero
Copy link
Contributor Author

/test e2e-aws-image-ecosystem

@gabemontero
Copy link
Contributor Author

ok the e2e-aws-proxy failure was an unrelated sig-arch failure @adambkaplan @coreydaley

all the sig-builds tests passed

samples come up as managed, no image import failures, and the pod logs indicated the expected behavior

this one is ready for any final review comments and tag for merge

@gabemontero
Copy link
Contributor Author

I'll

/retest

one more time to see if we get a total green, no unrelated flakes e2e-aws-proxy; otherwise I'll skip to facilitate merge

@gabemontero
Copy link
Contributor Author

low level install fail

/retest

@gabemontero
Copy link
Contributor Author

/assign @dperaza4dustbit

fyi - my last fix prior to ownership transfer broke OCP with proxy enabled .... this fixes it (the e2e-aws-proxy failures have been unrelated to this bug/fix, and I was able to verify in the must gather artifacts that samples was bootstrapping again as managed with proxy enabled as it should, where as the bug I introduced resulted in samples bootstrapping as removed).

})
if err != nil {

if len(proxy.Status.HTTPSProxy) > 0 || len(proxy.Status.HTTPProxy) > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Is there any chance that proxy.Status could be nil here? Or are we confident that h.configclient.Proxies.Get will provide that even if there is an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope it is not a pointer ... no nil ref issues here


proxy := &configv1.Proxy{}
var err error
wait.PollImmediate(5*time.Second, 20*time.Second, func() (bool, error) {
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be worth at least logging the timeout error from this showing that it never completed.

err := wait.PollImmediate(5*time.Second, 20*time.Second, func() (bool, error) {

proxy := &configv1.Proxy{}
var err error
Copy link
Member

Choose a reason for hiding this comment

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

nit: The error from inside of the wait.pollImmediate is never surfaced outside of that function, so this probably isn't needed here (but is for the second one below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually no with what I am doing with proxy I either need to do this as I have it, or I have to rename the var

leaving this as is

@coreydaley
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 24, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 24, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: coreydaley, gabemontero

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

@gabemontero
Copy link
Contributor Author

/skip

@gabemontero
Copy link
Contributor Author

/retest

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 24, 2021

@gabemontero: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/okd-e2e-aws 17b783a link false /test okd-e2e-aws
ci/prow/e2e-aws-proxy 17b783a link false /test e2e-aws-proxy
ci/prow/okd-e2e-aws-upgrade 17b783a link false /test okd-e2e-aws-upgrade
ci/prow/okd-e2e-aws-builds 17b783a link false /test okd-e2e-aws-builds

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@gabemontero
Copy link
Contributor Author

/retest

@gabemontero
Copy link
Contributor Author

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 27, 2021
@gabemontero
Copy link
Contributor Author

/bugzilla refresh

@openshift-ci openshift-ci bot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. and removed bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. labels Sep 27, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 27, 2021

@gabemontero: This pull request references Bugzilla bug 2006947, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.10.0) matches configured target release for branch (4.10.0)
  • bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @jitendar-singh

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-merge-robot openshift-merge-robot merged commit 156ab72 into openshift:master Sep 27, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 27, 2021

@gabemontero: All pull requests linked via external trackers have merged:

Bugzilla bug 2006947 has been moved to the MODIFIED state.

In response to this:

Bug 2006947: fix proxy portion of tbr inaccessible check

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants