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

allow build git/proxy e2e error msg parsing to work with multiple git… #24520

Merged

Conversation

gabemontero
Copy link
Contributor

… versions

Based on prototyping in openshift/builder#99 for using the https proxy friendly versions of scl git on rhel7, some slight adjustments to the existing build proxy related e2e error message parsing is needed to account for both the base version of git on rhel7 and the scl version,
as the error messages changed ever slow slightly.

Don't see non-message parsing alternatives to those tests. But of course suggestions welcome.

Of course merging this change does not mean we are committing to fully doing git https proxy support, but this should be a benign change that facilitates our testing when that work is officially taken on.

@openshift/openshift-team-developer-experience FYI

/assign @adambkaplan

@openshift-ci-robot
Copy link

@gabemontero: This pull request references Bugzilla bug 1750650, which is invalid:

  • expected the bug to be open, but it isn't
  • expected the bug to target the "4.4.0" release, but it targets "4.3.0" instead
  • expected the bug to be in one of the following states: NEW, ASSIGNED, ON_DEV, POST, POST, but it is CLOSED (DEFERRED) instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1750650: allow build git/proxy e2e error msg parsing to work with multiple git…

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-robot openshift-ci-robot added bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 7, 2020
@gabemontero gabemontero changed the title Bug 1750650: allow build git/proxy e2e error msg parsing to work with multiple git… allow build git/proxy e2e error msg parsing to work with multiple git… Feb 7, 2020
@gabemontero
Copy link
Contributor Author

Assuming we agree to merge this in the short term, I'll open a 4.4 bug for this.

@openshift-ci-robot openshift-ci-robot removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Feb 7, 2020
@openshift-ci-robot
Copy link

@gabemontero: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

In response to this:

allow build git/proxy e2e error msg parsing to work with multiple git…

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-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 7, 2020
@gabemontero
Copy link
Contributor Author

/retest

1 similar comment
@gabemontero
Copy link
Contributor Author

/retest

@gabemontero
Copy link
Contributor Author

fyi e2e-gcp-builds passed and that is the one affected by this e2e only PRs changes

@gabemontero
Copy link
Contributor Author

OK @adambkaplan we have green tests

open a bug and get into 4.4, or wait until 4.5 ?

Copy link
Contributor

@adambkaplan adambkaplan left a comment

Choose a reason for hiding this comment

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

/lgtm

/hold

IMO swapping out git with the new SCL version is a high-risk fix. Let's land it in 4.5 so we have plenty of soak time.

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Feb 11, 2020
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan, 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

/lgtm

/hold

IMO swapping out git with the new SCL version is a high-risk fix. Let's land it in 4.5 so we have plenty of soak time.

Agree on the risk @adambkaplan

Just to be clear, this PR is just the e2e tweaks to accommodate old and new git versions. That is why I even considered merging this now.

The actual change is going to be a combination of library-go and builder changes. Those changes will need this tweak to merge.

@gabemontero
Copy link
Contributor Author

@adambkaplan with 4.5 now open are you good with unholding this PR

Reminder: this only enables support for parsing git messages in our e2e's for both the current older version's message, as well as the new SCL version IF we choose to pull that in to address the current HTTPS proxy limitation with git (via its dependencies)

thanks

@ricardomaraschini FYI ^^

@adambkaplan
Copy link
Contributor

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 20, 2020
@gabemontero
Copy link
Contributor Author

terraform flake on serial failure

/test e2e-aws-serial

@openshift-bot
Copy link
Contributor

/retest

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

4 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

3 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@gabemontero
Copy link
Contributor Author

quick update - none of the e2e churn has been with the git test cases tweaked by this PR, and those tests did pass a while back with https://openshift-gce-devel.appspot.com/build/origin-ci-test/pr-logs/pull/24520/pull-ci-openshift-origin-master-e2e-gcp-builds/1075/

@gabemontero
Copy link
Contributor Author

Keeping an eye on e2e-gcp-builds to see if new unrelated flakes are with us.

@openshift-bot
Copy link
Contributor

/retest

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

@gabemontero
Copy link
Contributor Author

gabemontero commented Feb 21, 2020

/hold

I think we may have something in 4.5 changing such that our build cluster configuration tests are failing @adambkaplan

https://prow.svc.ci.openshift.org/view/gcs/origin-ci-test/pr-logs/pull/24520/pull-ci-openshift-origin-master-e2e-gcp-builds/1206 is the latest incarnation

Have you seen/heard anything?

I'll try to triage the above run in a bit ... gonna wait to see how the current run does. But we've had a couple of similar failures in a row now.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 21, 2020
@adambkaplan
Copy link
Contributor

@gabemontero I think those disruptive tests are broken by a fix I made in 4.5. #24582 should hopefully fix this

@gabemontero
Copy link
Contributor Author

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 22, 2020
@gabemontero
Copy link
Contributor Author

/retest

@openshift-bot
Copy link
Contributor

/retest

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

5 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

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. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants