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

test/extended/builds: Setting Build Defaults should has effect on build pod #24717

Merged
merged 1 commit into from Apr 7, 2020

Conversation

wewang58
Copy link
Contributor

@openshift/devexp-qe Auto case: OCP-24372
/assign @adambkaplan
cc @gabemontero
Thanks all
passed in local: http://pastebin.test.redhat.com/845755

@openshift-ci-robot
Copy link

@wewang58: This pull request references Bugzilla bug 1730719, which is invalid:

  • expected the bug to be open, but it isn't
  • expected the bug to target the "4.5.0" release, but it targets "4.2.0" instead
  • expected the bug to be in one of the following states: NEW, ASSIGNED, ON_DEV, POST, POST, but it is CLOSED (ERRATA) 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 1730719: text/extended/builds: Setting Build Defaults should has effect on build pod

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 bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Mar 18, 2020
@wewang58 wewang58 changed the title Bug 1730719: text/extended/builds: Setting Build Defaults should has effect on build pod test/extended/builds: Setting Build Defaults should has effect on build pod Mar 18, 2020
@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 Mar 18, 2020
@openshift-ci-robot
Copy link

@wewang58: 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:

test/extended/builds: Setting Build Defaults should has effect on build pod

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.

@wewang58
Copy link
Contributor Author

@gabemontero @adambkaplan When you have time, could you give advice about the pr about Setting Build Defaults should has effect on build pod, thanks a lot!

@gabemontero
Copy link
Contributor

@gabemontero @adambkaplan When you have time, could you give advice about the pr about Setting Build Defaults should has effect on build pod, thanks a lot!

hey @wewang58 ... ultimately we need to defer to @adambkaplan on this, but some observations to try and help things along

  1. the e2e-aws-fips, e2e-gcp, and e2e-gcp-upgrade failures are not related to your test cases updates
  2. if you do not see them in some of the new prow/deck search capabilities that Clayton and Ben have been working on (see "FEAT: search.svc.ci.openshift.org now searches bugs and bug comments" on aos-devel and example queries like https://search.svc.ci.openshift.org/?search=buildcop&maxAge=48h&context=2&type=bug# )
  3. I don't see your tests actually in e2e-gcp-builds or e2e-aws-serial (though there appears to be a junit collection hiccup in the last e2e-aws-serial run); we'll want to confirm actual execution before merging ... that might mean re-running those successful tests again, or ...
  4. I know @adambkaplan had been making some changes recently with the cluster config tests and the OCM rollout stuff ... so a) he would be best to make sure those elements of your new test are adhering to his latest approaches, b) maybe he can better speak to which suite your test should land in

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.

@wewang58 a few small asks, otherwise is in good shape (and passed on the builds CI run)

test/extended/builds/cluster_config.go Show resolved Hide resolved
foundRequestCpu := false
foundRequestMem := false
for _, container := range containers {
o.Expect(container.Resources).NotTo(o.BeNil())
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add an o.By() call here so we know which container doesn't have the right spec:

o.By(fmt.Sprintf("checking resources of container %s", container.Name))

Copy link
Contributor Author

@wewang58 wewang58 Mar 24, 2020

Choose a reason for hiding this comment

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

Yes, it's more clear to add this, and Updated. Thanks! @adambkaplan

@wewang58
Copy link
Contributor Author

@gabemontero @adambkaplan When you have time, could you give advice about the pr about Setting Build Defaults should has effect on build pod, thanks a lot!

hey @wewang58 ... ultimately we need to defer to @adambkaplan on this, but some observations to try and help things along

1. the e2e-aws-fips, e2e-gcp, and e2e-gcp-upgrade failures are not related to your test cases updates

2. if you do not see them in some of the new prow/deck search capabilities that Clayton and Ben have been working on (see "FEAT: search.svc.ci.openshift.org now searches bugs and bug comments" on aos-devel and example queries like https://search.svc.ci.openshift.org/?search=buildcop&maxAge=48h&context=2&type=bug# )

3. I don't see your tests actually in e2e-gcp-builds or e2e-aws-serial (though there appears to be a junit collection hiccup in the last e2e-aws-serial run);  we'll want to confirm actual execution before merging ... that might mean re-running those successful tests again, or ...

4. I know @adambkaplan had been making some changes recently with the cluster config tests and the OCM rollout stuff ... so a) he would be best to make sure those elements of your new test are adhering to his latest approaches, b) maybe he can better speak to which suite your test should land in

Thanks @gabemontero, Now I know more workflow about submit test pr.

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

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 25, 2020
@openshift-bot
Copy link
Contributor

/retest

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

13 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.

@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.

@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.

15 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.

@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.

@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.

@wewang58
Copy link
Contributor Author

wewang58 commented Apr 5, 2020

/hold
@adambkaplan Seems ci failed to build openshift-tests binary, all tests failed, cause robot always retest, I hold the pr temporary.

@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 Apr 5, 2020
@adambkaplan
Copy link
Contributor

@wewang58 rebase hit. You need to rebase your PR against current master, and may need to make a few changes to your test. All clients now take a context.Context parameter.

…lts (requests/limits) should has effect on build pod
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 7, 2020
@wewang58
Copy link
Contributor Author

wewang58 commented Apr 7, 2020

@adambkaplan Now almost all jobs success, could you help to add lgtm label again, after git rebase, the label removed, thanks a lot.
/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 Apr 7, 2020
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

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan, wewang58

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 lgtm Indicates that a PR is ready to be merged. label Apr 7, 2020
@openshift-bot
Copy link
Contributor

/retest

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

2 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-merge-robot openshift-merge-robot merged commit 502a7b6 into openshift:master Apr 7, 2020
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants