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

fixes(#598): checks for namespace creation instead of waiting #611

Merged
merged 3 commits into from
Jan 24, 2020

Conversation

maximilien
Copy link
Contributor

Fixes #598

Proposed Changes

  • checks for namespace creation instead of waiting

CHANGELOG.adoc:

  • 🧽 Update or clean up current behaviour

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jan 20, 2020
@knative-prow-robot knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 20, 2020
@maximilien
Copy link
Contributor Author

/ok-to-test

@knative-prow-robot knative-prow-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jan 20, 2020
@rhuss
Copy link
Contributor

rhuss commented Jan 21, 2020

CHANGELOG.adoc:

* 🧽 Update or clean up current behaviour

Thanks for giving the CHANGELOG some love here, but it would be easier for me (or whoever makes the next release) if you would add this directory to CHANGELOG.adoc and add it as part of your PR.

@maximilien
Copy link
Contributor Author

CHANGELOG.adoc:

* 🧽 Update or clean up current behaviour

Thanks for giving the CHANGELOG some love here, but it would be easier for me (or whoever makes the next release) if you would add this directory to CHANGELOG.adoc and add it as part of your PR.

Not sure I understand this request... I essentially selected that line when submitting the PR. This comes from the template. Am I missing something?

@maximilien
Copy link
Contributor Author

@navidshaikh and @rhuss can I get a LGTM here. This is done and ready and increases the speed of e2e in appreciable manner. Thx.

Copy link
Contributor

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

looks good, but please reduce the retry timeout otherwise the situation get even worse wrt/ to e2e duration.

@rhuss
Copy link
Contributor

rhuss commented Jan 23, 2020

Not sure I understand this request... I essentially selected that line when submitting the PR. This comes from the template. Am I missing something?

Sorry, didn't state that clearly. Actually I would like to change the template to mention that this line should be directly added into the CHANGELOG.adoc file instead (and add it to the PR files) of just a comment in the PR. Because otherwise still someone need to pick that up and copy it over, which actually is not of much help as one has to go over every PR. At the moment we still have to do it, but for the future it would be really helpful.

No worries here, but I will update the template to add a note.

@rhuss
Copy link
Contributor

rhuss commented Jan 23, 2020

@maximilien well, I just saw that this is already mentioned in the template:

<!--
Release Note:

In the following cases, add a short description of PR to the unreleased section in CHANGELOG.adoc:

- 🎁 Add new feature
- 🐛 Fix bug
- 🧽 Update or clean up current behaviour
- 🗑️ Remove feature or internal logic

See other entries in CHANGELOG.adoc as an example.

PLEASE DON'T ADD THAT LINE HERE IN THE PULL-REQUEST DESCRIPTION BUT DIRECTLY IN CHANGELOG.ADOC AND ADD CHANGELOG.ADOC AS PART OF YOUR PULL-REQUEST.
-->

Do you have a suggestion how to state it more clearly ?

@navidshaikh
Copy link
Collaborator

I tested the PR locally and works fine.

There are few improvements we can bring to e2e tests setup/teardown in a subsequent PR,
for example: parsing the error while creating the test namespace, if it AlreadyExists,
we can delete and recreate it.

@maximilien
Copy link
Contributor Author

@maximilien well, I just saw that this is already mentioned in the template:

<!--
Release Note:

In the following cases, add a short description of PR to the unreleased section in CHANGELOG.adoc:

- 🎁 Add new feature
- 🐛 Fix bug
- 🧽 Update or clean up current behaviour
- 🗑️ Remove feature or internal logic

See other entries in CHANGELOG.adoc as an example.

PLEASE DON'T ADD THAT LINE HERE IN THE PULL-REQUEST DESCRIPTION BUT DIRECTLY IN CHANGELOG.ADOC AND ADD CHANGELOG.ADOC AS PART OF YOUR PULL-REQUEST.
-->

Do you have a suggestion how to state it more clearly ?

Ah, OK. Make sense :) Thanks and will update and reduce the inter wait timeout. Cheers 🍻

@knative-prow-robot knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 24, 2020
Copy link
Contributor

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

Thanks !

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 24, 2020
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maximilien, rhuss

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

@knative-prow-robot knative-prow-robot merged commit 606de80 into knative:master Jan 24, 2020
@maximilien maximilien deleted the issue598 branch March 27, 2020 03:18
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. cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Could e2e tests run faster?
6 participants