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

creating cowsay kuttl tests #284

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

creating cowsay kuttl tests #284

wants to merge 2 commits into from

Conversation

kensipe
Copy link
Member

@kensipe kensipe commented Jul 24, 2020

Signed-off-by: Ken Sipe [email protected]

Copy link
Member

@nfnt nfnt left a comment

Choose a reason for hiding this comment

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

Where is this being tested? Whouldn't we need to add ./repository/cowsay/tests to testDirs in kuttl-test.yaml?

status:
phase: Running

# confirms that kudo is running
Copy link
Member

Choose a reason for hiding this comment

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

Why is it necessary to check this here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I also thought that this PR removed this assertion in favor of kudo init ... --wait

Copy link
Member Author

Choose a reason for hiding this comment

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

it makes the test more portal IMO... yes we should have a kudo init --wait. This fails fast if that wasn't done.. and I see these tests as something we may point users to... that may run them...

Copy link
Contributor

@zen-dog zen-dog Aug 3, 2020

Choose a reason for hiding this comment

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

I mean, we don't wait for KUDO this way for other operators tests today (because we already kudo init --wait) so I'd rather stay consistent right now and remove this. The inconsistency concern is IMO bigger than portability one.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @zen-dog here - This is testing something that should be ensured globally not in each test. If we have this here, it will be copied to other tests and we end up with having this step duplicated everywhere.

We had this in the kudo e2e tests as well, I was glad we could remove it there.

Copy link
Member

@zmalik zmalik left a comment

Choose a reason for hiding this comment

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

the intention here isn't not to run these tests with each PR but in a later iteration, we can work on when to run these tests and how to optimize running tests only for operator which is being changed.

I would like to see at least a weekly build or nightly with all the tests present in this repository, otherwise, we will have only broken tests with time. I will add this as a topic for the next community meeting

@@ -0,0 +1,15 @@
apiVersion: v1
binaryData:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to check the exact content of the config map. Its presence along with the name: cowsayinstance.deploy.main.genfiles.genwww.indexhtml should be enough.

Copy link
Member Author

Choose a reason for hiding this comment

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

yep... totally makes sense... removed

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.

lgtm, although I would prefer to not have the kudo assert as the first step.

status:
phase: Running

# confirms that kudo is running
Copy link
Member

Choose a reason for hiding this comment

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

I agree with @zen-dog here - This is testing something that should be ensured globally not in each test. If we have this here, it will be copied to other tests and we end up with having this step duplicated everywhere.

We had this in the kudo e2e tests as well, I was glad we could remove it there.

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.

5 participants