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

KEP-0007: Assertions for CLI commands #245

Merged
merged 5 commits into from
Dec 14, 2020
Merged

Conversation

nfnt
Copy link
Member

@nfnt nfnt commented Oct 28, 2020

What this PR does / why we need it:
By asserting CLI commands, kuttl can support many additional test scenarios that examining the internal state of an operator.

Copy link
Member

@ANeumann82 ANeumann82 left a comment

Choose a reason for hiding this comment

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

Nice work :) Some open questions though

keps/0007-command-asserts.md Outdated Show resolved Hide resolved
keps/0007-command-asserts.md Outdated Show resolved Hide resolved
Comment on lines 69 to 104
```yaml
apiVersion: kuttl.dev/v1beta1
type: TestAssert
commands:
- command: ./parse-logs.sh name-of-pod-0 string-to-search-for
namespaced: true
```
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
```yaml
apiVersion: kuttl.dev/v1beta1
type: TestAssert
commands:
- command: ./parse-logs.sh name-of-pod-0 string-to-search-for
namespaced: true
```
```yaml
apiVersion: kuttl.dev/v1beta1
type: TestAssert
commands:
- command: ./parse-logs.sh name-of-pod-0 string-to-search-for
namespaced: true
timeout: 60
delay: 10
I would suggest an addition of an "assertDelay". This is currently fixed at 1 second, meaning the assert is checked after a delay of 1 second. 
When we add commands, we might want to increase this delay, maybe because of very expensive checks. 
An `initialDelay` might also make sense, although adding delays to check is often bad design...

Copy link
Member

Choose a reason for hiding this comment

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

not a fan of delay... If we have one... we should be clear a delay from what... I would rather have the declarative asserts successful as a criteria of when to start commands... time delays are a common source of flaky tests :(

Copy link
Member

Choose a reason for hiding this comment

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

Well, we currently have a fixed delay of 1 second between rechecks. I.e. when an assert fails, there is a one-second delay until it is checked again. Might be worth to make that configurable.

keps/0007-command-asserts.md Show resolved Hide resolved
Copy link
Member

@kensipe kensipe left a comment

Choose a reason for hiding this comment

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

Lets have a round of discussions and edits... love that this moving! thank you!

keps/0007-command-asserts.md Outdated Show resolved Hide resolved
keps/0007-command-asserts.md Outdated Show resolved Hide resolved
keps/0007-command-asserts.md Show resolved Hide resolved
keps/0007-command-asserts.md Show resolved Hide resolved
#### User Story 2

An operator can be configured to encrypt it's API. To assert that the API endpoints are indeed encrypted we can run a specific `curl` command and check that it returns successfully once the operator has been deployed.

Copy link
Member

Choose a reason for hiding this comment

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

it might be interesting to parse the test logs... I'm not sure how that would be done.. but we have the ability to have background processes which are used for in development operators... you may want to parse the controller output which is in the test stdout.

If not... we should add this to the list of future considerations at the bottom of this kep

keps/0007-command-asserts.md Show resolved Hide resolved
keps/0007-command-asserts.md Show resolved Hide resolved
keps/0007-command-asserts.md Show resolved Hide resolved
Copy link
Member

@kensipe kensipe left a comment

Choose a reason for hiding this comment

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

worth considering "shouldFail" for commands... and lets add it to a goal or non-goal and future considerations if not a current goal.

#146

Copy link
Member

@ANeumann82 ANeumann82 left a comment

Choose a reason for hiding this comment

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

I'm generally ok with the KEP - I still think it can be extended quite a bit, but that doesn't need to be done in the first implementation.

I'd like to see a couple of kens questions answered somwhere in the KEP, for example things about log output if a command fails, if the report output changes, and if so how.

@nfnt nfnt removed their assignment Nov 9, 2020
@kensipe kensipe self-assigned this Nov 11, 2020
Jan Schlicht and others added 5 commits December 14, 2020 11:48
By asserting CLI commands, kuttl can support many additional test scenarios that examing the internal state of an operator.

Signed-off-by: Jan Schlicht <jan@d2iq.com>
Signed-off-by: Jan Schlicht <jan@d2iq.com>
Signed-off-by: Jan Schlicht <jan@d2iq.com>
Signed-off-by: Jan Schlicht <jan@d2iq.com>
Signed-off-by: Ken Sipe <kensipe@gmail.com>
@kensipe kensipe force-pushed the nfnt/kep-0007-command-asserts branch from 7ad6fa1 to ddf3085 Compare December 14, 2020 17:48
@kensipe
Copy link
Member

kensipe commented Dec 14, 2020

rebased to main

@kensipe kensipe merged commit 1376c32 into main Dec 14, 2020
@kensipe kensipe deleted the nfnt/kep-0007-command-asserts branch December 14, 2020 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants