Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Adding e2e make target, makefile adjustments, and e2e test framework #81
Adding e2e make target, makefile adjustments, and e2e test framework #81
Changes from all commits
3e18957
1d621ba
6858e1d
e60478f
f997bfd
dce6b5a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Looking at consistency, we might want to rename these. If we wanted to go with k8s' V-O verbiage:
e.g.
kind-cluster
->kind-create-cluster
kind-cluster-cleanup
->kind-delete-cluster
kind-load
->kind-load-image
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with your reasoning for these, but I also named them this way for the sake of consistency with rukpak, so we'd need to change them in both places if we also wanted to be internally consistent. If it's alright with you, I'd prefer to leave these at least for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
test -s $(LOCALBIN)/<binName>
tests are sufficient for CI because they'll always fail and we'll always get the correct version.But locally, these can be annoying when we bump the version of one of these and you still have an old version from an earlier commit. If there's some diff in behavior between your local version and the one specified here, you'll be scratching your head trying to figure out why.
There are two solutions I've seen for this:
tools
go module with a separate go.mod that lists all the tool versions with make targets that alwaysgo build
Option 1 is good if all of our tools are Go-based
Option 2 is probably slightly faster when actually using the tools, but requires some tweaks every now and then when a tool changes its release artifacts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be a follow-up for sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did notice number 1 there in rukpak's makefile. I initially was working with that but it ended up turning into a bit of a rabbit-hole with the amount of changes needed, since it appeared as though it would require re-working every built-in make target we got from kubebuilder, and then at that point it felt like I was going to end up with just a copy-paste of rukpak's makefile which would require re-testing all of the targets. We definitely need to take care of it at some point, if you think it's a good thing to take care of now I could give it another go. It just felt like it was getting a bit out-of-scope to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've created issue #85 to track this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could these tests go under suite_test.go? Just because we have the test framework already scaffolded by the tool?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just saw Joe's comment #81 (comment), please feel free to ignore my review :)
Only one nit (we could do in a follow up), we could use envtest even for the unit (case 1) tests (though it is mainly intended for integration) if we foresee the interaction of multiple individual components, and would have to test "Reconcile" anyway. This in the case (1) where we need deppy entity source, and essentially mock the functioning of controller. This is just because previously we had faced version incompatibility issues by using locally available binaries and had a hard time figuring the cause.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Varsha! I have to admit, my knowledge on the envtest stuff is lacking here, so if it's alright with you I'd like to reserve that for a follow up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, I have created an issue to follow up #86. We can start using envtest when we get to writing tests for reconciler depending on the complexity of those. Till then its good :)