Skip to content

Update writing-e2e-tests doc #2026

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

Conversation

pratikjagrut
Copy link
Contributor

Description of the change:
Adding one-liner information about --namespace flag.

Motivation for the change:
I was under the impression that operator-sdk test local ./test/e2e --up-local --namespace=memcached will create namespece=memcached and when I tried the command it was failing due to namespaces "memcached" not found and for this, I raised an issue #2014. But later I got to know that it won't create any namespace but we need to specify the namespace that it should watch for changes in. So I thought it's a good piece of information and should have a place in the doc.

@camilamacedo86 camilamacedo86 self-assigned this Oct 9, 2019
**NOTE**: The command will NOT create the `--namespace`. Then, be sure that you are specifying a valid namespace.
resources unless the user specifies one through the `--namespaced-manifest` flag. (**NOTE**: The `--up-local` flag requires
the `--namespace` flag,
and the command will NOT create the `namespace`, then, be sure that you are specifying a valid namespace).
Copy link
Contributor

Choose a reason for hiding this comment

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

I think is too long the sentence, also IMHO it would be better bellow the phase as a NOTE in the doc. WDYT could we change it?

Copy link
Contributor Author

@pratikjagrut pratikjagrut Oct 9, 2019

Choose a reason for hiding this comment

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

Note: the --up-local flag requires the --namespace flag: NOTE: The command will NOT create the --namespace. Then, be sure that you are specifying a valid namespace.
Are you suggesting something like this?
Wouldn't it be the note followed by a note, which extends first note?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am suggesting:

  • Do not perform changes in the current explanation
  • Then, just add a NOTE: in the next line and not in the same paragraph.

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.....(Note: the --up-local flag requires the --namespace flag).

NOTE: The command will NOT create the namespace. Then, be sure that you are specifying a valid namespace.

Something like this!
This is doable, and yeah more elegant.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is in the same line which is not the docs standards. The note in the same line with () makes harder the understanding.

See the following example.

**NOTE**: A unit test checking more cases can be found in our [`samples repo`][code-test-example].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so I should remove the existing note from the paragraph and add a new note to the next line but not in the same paragraph.
paragraph...

**NOTE**: The --up-local flag requires the --namespace flag and the command will NOT create the --namespace. Then, be sure that you are specifying a valid namespace

Something like this! This was your code suggestion!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Committed changes have a look if its OK or not?

Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

👍 great!
Tks for the contribution.

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 9, 2019
@camilamacedo86
Copy link
Contributor

/test marker

@camilamacedo86
Copy link
Contributor

/test e2e-aws-go

@camilamacedo86
Copy link
Contributor

Very small nit NOTE in the docs. So, I am merging it.

@camilamacedo86 camilamacedo86 merged commit a3645e3 into operator-framework:master Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

3 participants