Skip to content

Comments

Define all flags before parsing flags#161

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
alvaroaleman:fix-flagset
Apr 6, 2022
Merged

Define all flags before parsing flags#161
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
alvaroaleman:fix-flagset

Conversation

@alvaroaleman
Copy link
Contributor

@alvaroaleman alvaroaleman commented Apr 5, 2022 •
edited
Loading

Currently, consumers that use the --apigroup flag are being broken, because it is defined after the flagparsing:

$ k logs machine-approver-d8964878b-vdlsk 
unknown flag: --apigroup

For a version that is on 6c68c2e

skopeo inspect docker://registry.ci.openshift.org/ocp/4.11-2022-04-03-041227@sha256:0a96d3f0c672eb31ef0adcda20665a4426bb5e49c3fb1ef5f81d6a5646a00586|rg vcs-ref
        "vcs-ref": "6c68c2ee5a8c14cb375f895dea9fd03ebeebf675",

This regression was introduced by #157

@openshift-ci openshift-ci bot requested review from damdo and elmiko April 5, 2022 16:11
@enxebre
Copy link
Member

enxebre commented Apr 5, 2022

Can you please update the PR desc clarifying this breaking consumers of these flags atm i.e hypershift
/lgtm

PTAL @JoelSpeed @elmiko

@JoelSpeed
Copy link
Contributor

JoelSpeed commented Apr 5, 2022

Would it be possible to attach a BZ to this as well, I'm assuming this broke hypershift testing?
Would also be good to include a link to the PR that introduced the regression

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 5, 2022
@alvaroaleman
Copy link
Contributor Author

alvaroaleman commented Apr 5, 2022

@JoelSpeed updated the description. What is the value of creating a BZ when there is already a fix and the bug only exists in an unreleased version?

@JoelSpeed
Copy link
Contributor

JoelSpeed commented Apr 5, 2022

I wasn't sure it was just in the current unreleased version, never mind about the BZ, LGTM
/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 5, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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 openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 5, 2022
elmiko
elmiko reviewed Apr 5, 2022
View reviewed changes
Copy link
Contributor

@elmiko elmiko left a comment

Choose a reason for hiding this comment

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

/lgtm

@alvaroaleman
Copy link
Contributor Author

alvaroaleman commented Apr 5, 2022

msg=failed to fetch Cluster: failed to generate asset "Cluster": failed to create cluster: failed to apply Terraform: exit status 1

Unlikely to be related to the change
/retest-required

@alvaroaleman
Copy link
Contributor Author

alvaroaleman commented Apr 5, 2022

/retest-required

@openshift-bot
Copy link
Contributor

openshift-bot commented Apr 5, 2022

/retest-required

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link
Contributor

openshift-bot commented Apr 5, 2022

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Apr 5, 2022

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@alvaroaleman
Copy link
Contributor Author

alvaroaleman commented Apr 5, 2022

/retest-required

@openshift-bot
Copy link
Contributor

openshift-bot commented Apr 5, 2022

/retest-required

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link
Contributor

openshift-bot commented Apr 5, 2022

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Apr 6, 2022

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@sjenning
Copy link
Contributor

sjenning commented Apr 6, 2022

seems to be a very flakey test suite 😖
https://prow.ci.openshift.org/job-history/gs/origin-ci-test/pr-logs/directory/pull-ci-openshift-cluster-machine-approver-master-e2e-aws-capi-techpreview

@openshift-bot
Copy link
Contributor

openshift-bot commented Apr 6, 2022

/retest-required

Please review the full test history for this PR and help us cut down flakes.

4 similar comments
@openshift-bot
Copy link
Contributor

openshift-bot commented Apr 6, 2022

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Apr 6, 2022

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Apr 6, 2022

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Apr 6, 2022

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@sjenning sjenning mentioned this pull request Apr 6, 2022
Merged
@alvaroaleman
Copy link
Contributor Author

alvaroaleman commented Apr 6, 2022

/override e2e-aws-capi-techpreview

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 6, 2022

@alvaroaleman: alvaroaleman unauthorized: /override is restricted to Repo administrators, approvers in top level OWNERS file.

Details

In response to this:

/override e2e-aws-capi-techpreview

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.

@alexander-demicev
Copy link
Contributor

alexander-demicev commented Apr 6, 2022

/override e2e-aws-capi-techpreview

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 6, 2022

@alexander-demichev: /override requires a failed status context or a job name to operate on.
The following unknown contexts were given:

  • e2e-aws-capi-techpreview

Only the following contexts were expected:

  • ci/prow/e2e-aws
  • ci/prow/e2e-aws-capi-techpreview
  • ci/prow/e2e-aws-disruptive
  • ci/prow/e2e-aws-operator
  • ci/prow/e2e-azure-operator
  • ci/prow/e2e-gcp-operator
  • ci/prow/e2e-upgrade
  • ci/prow/images
  • ci/prow/unit
  • pull-ci-openshift-cluster-machine-approver-master-e2e-aws
  • pull-ci-openshift-cluster-machine-approver-master-e2e-aws-capi-techpreview
  • pull-ci-openshift-cluster-machine-approver-master-e2e-aws-disruptive
  • pull-ci-openshift-cluster-machine-approver-master-e2e-aws-operator
  • pull-ci-openshift-cluster-machine-approver-master-e2e-azure-operator
  • pull-ci-openshift-cluster-machine-approver-master-e2e-gcp-operator
  • pull-ci-openshift-cluster-machine-approver-master-e2e-upgrade
  • pull-ci-openshift-cluster-machine-approver-master-images
  • pull-ci-openshift-cluster-machine-approver-master-unit
  • tide
Details

In response to this:

/override e2e-aws-capi-techpreview

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.

@alexander-demicev
Copy link
Contributor

alexander-demicev commented Apr 6, 2022

/override ci/prow/e2e-aws-capi-techpreview

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 6, 2022

@alexander-demichev: Overrode contexts on behalf of alexander-demichev: ci/prow/e2e-aws-capi-techpreview

Details

In response to this:

/override ci/prow/e2e-aws-capi-techpreview

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
Copy link
Contributor

openshift-ci bot commented Apr 6, 2022

@alvaroaleman: 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/e2e-aws-disruptive f356e22 link false /test e2e-aws-disruptive
ci/prow/e2e-azure-operator f356e22 link false /test e2e-azure-operator
ci/prow/e2e-gcp-operator f356e22 link false /test e2e-gcp-operator

Full PR test history. Your PR dashboard.

Details

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.

@openshift-merge-robot openshift-merge-robot merged commit b6cc85c into openshift:master Apr 6, 2022
@alvaroaleman alvaroaleman deleted the fix-flagset branch April 6, 2022 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@damdo damdo Awaiting requested review from damdo

1 more reviewer

@elmiko elmiko elmiko left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@elmiko elmiko

@enxebre enxebre

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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@alvaroaleman @enxebre @JoelSpeed @openshift-bot @sjenning @alexander-demicev @elmiko @openshift-merge-robot