Skip to content
This repository was archived by the owner on Jun 19, 2022. It is now read-only.

[WIP]: Add object naming conflicts e2e test#1995

Closed
cathyzhyi wants to merge 4 commits into
google:mainfrom
cathyzhyi:clusterrole_test
Closed

[WIP]: Add object naming conflicts e2e test#1995
cathyzhyi wants to merge 4 commits into
google:mainfrom
cathyzhyi:clusterrole_test

Conversation

@cathyzhyi
Copy link
Copy Markdown
Contributor

Fixes #1994

Proposed Changes

  • Refactor test/lib.sh to separate out knative-gcp installation
  • Add shell executor to utils
  • Add object naming conflict e2e test

Release Note


Docs

@knative-prow-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cathyzhyi

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

@google-cla google-cla Bot added the cla: yes (override cla status due to multiple authors bug) label Dec 14, 2020
@cathyzhyi
Copy link
Copy Markdown
Contributor Author

/hold
Hold merge to let #1917 be merged first and address the cloud-run-events naming related parts in this PR

Comment thread test/lib/utils.go
return value
}

func CallShellFunctionAndGetStdout(funcName, scriptNameWithPath, projectLocation string, out io.Writer) error {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

FWIW, this will not run outside knative* repos.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

hmm, after this knative/hack#32 the places that are allowed to use the shell util are narrowed to only upgrade test and knative/hack/shell.

Comment thread test/conflict/main_test.go Outdated
// If the last word is "configured", there is a naming conflict with the components already installed
pattern := "configured"
var prevline string
if strings.Contains(out.String(), pattern) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

To be safe, you can just iterate per line, and check if it ends wth "configured."

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Do you mean it's safer to always check each line rather than do an overall check first?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Based on the comment, you want to make sure it is the last word. Otherwise you will match sth like: "We configured this as ....".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point! Changed as suggested.

pattern := "configured"
for _, line := range strings.Split(out.String(), "\n") {
words := strings.Fields(line)
if len(words) > 0 && strings.Compare(words[len(words)-1], pattern) == 0 {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

string.HasSufix?

@knative-prow-robot
Copy link
Copy Markdown
Contributor

@cathyzhyi: PR needs rebase.

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.

Base automatically changed from master to main February 5, 2021 05:12
@knative-prow-robot
Copy link
Copy Markdown
Contributor

@cathyzhyi: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-google-knative-gcp-build-tests 9b24996 link /test pull-google-knative-gcp-build-tests
pull-google-knative-gcp-unit-tests 9b24996 link /test pull-google-knative-gcp-unit-tests
pull-google-knative-gcp-integration-tests 9b24996 link /test pull-google-knative-gcp-integration-tests
pull-google-knative-gcp-wi-tests 9b24996 link /test pull-google-knative-gcp-wi-tests
pull-google-knative-gcp-upgrade-tests 9b24996 link /test pull-google-knative-gcp-upgrade-tests
pull-google-knative-gcp-conformance-tests 9b24996 link /test pull-google-knative-gcp-conformance-tests
pull-google-knative-gcp-go-coverage 9b24996 link /test pull-google-knative-gcp-go-coverage

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.

@eclipselu
Copy link
Copy Markdown
Contributor

@cathyzhyi please rebase this PR, thanks!

@cathyzhyi cathyzhyi changed the title Add object naming conflicts e2e test WIP: Add object naming conflicts e2e test Feb 16, 2021
@cathyzhyi cathyzhyi changed the title WIP: Add object naming conflicts e2e test [WIP]: Add object naming conflicts e2e test Feb 18, 2021
@cathyzhyi cathyzhyi closed this May 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add e2e test for detecting naming conflicts between knative-gcp and other components

4 participants