-
Notifications
You must be signed in to change notification settings - Fork 41
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
setup e2e using the cluster-api provided framework #49
Conversation
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.
Two questions:
- The Makefile got reformatted, it probably won't work correctly. Please fix.
- Why is there a separate
hack/tools/go.mod
andhack/tools/go.sum
? Did we install some tool and accidentally get that?
Fixed, fat fingers
This is the repository cluster-api folks gave to me as reference |
This is the code that starts a kind cluster that can be used to e2e tests. We are blocked by using the "Getting Started" scenario they provide because it uses KubeadmControlPlane
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.
Minor naming / var in Makefile
Makefile
Outdated
e2e: | ||
# This is the name used inside the component.yaml for the container that runs the manager | ||
# The image gets loaded inside kind from ./test/e2e/config/packet-dev.yaml | ||
docker tag controller:latest packethost/cluster-api-packet-controller |
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 should be docker tag $(IMG) $(BUILD_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.
We already have controller:latest
set as a var; let's fix it.
OK, I see what you are doing. I actually like this. We may need to do some future cleanups, but this is pretty good! |
|
||
.PHONY: ginkgo | ||
ginkgo: | ||
cd $(TOOLS_DIR) && go build -tags=tools -o $(BIN_DIR)/ginkgo github.com/onsi/ginkgo/ginkgo |
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 would remove this from .PHONY
, make the target be like this:
ginkgo: $(GINKGO)
$(GINKGO):
cd $(TOOLS_DIR) && go build -tags-tools -o $@ github.com/onsi/ginkgo/ginkgo
This takes advantages of the makefile dependencies.
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.
Missed this?
contract: CONTRACT | ||
- major: 0 | ||
minor: 3 | ||
contract: v1alpha3 |
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.
Ouch. Why did we change this? We are parsing it as part of our make managerless
and make release
, so it remains generic. Otherwise we have to maintain it here and in the Makefile
, guaranteed to get out of data.
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.
mmm it was not working in the way I setup e2e in test/e2e/config/packet-dev.conf.. Not sure how to fix it yet, if it has to be fixed. Any tips for 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.
This file is our own file, nothing "official" references it, so whatever uses it should be ours.
What consumes it?
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.
OK, I see it here:
files:
- sourcePath: "../../../templates/metadata-template.yaml"
targetName: "metadata.yaml"
- sourcePath: "../../../templates/cluster-template.yaml"
What does this do? Can it have a replacements thing like the section before it?
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 it can not, the only acceptable fields are sourcePath
and targetName
for files
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.
That yaml file is full of hard-coded versions; this is going to be painful to maintain.
Let's not hold this up to get those fixed, so let's do the following:
- Merge this in
- Open a new PR with a new doc in
docs/VERSIONS.md
that lists everywhere that versions are maintained, so at least someone knows about them - Open a new issue that we need to templatize all of these versions and make that new doc much simpler
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.
We are bound by their limitations, so let's work with it for now
contract: CONTRACT | ||
- major: 0 | ||
minor: 3 | ||
contract: v1alpha3 |
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.
That yaml file is full of hard-coded versions; this is going to be painful to maintain.
Let's not hold this up to get those fixed, so let's do the following:
- Merge this in
- Open a new PR with a new doc in
docs/VERSIONS.md
that lists everywhere that versions are maintained, so at least someone knows about them - Open a new issue that we need to templatize all of these versions and make that new doc much simpler
Sounds good! |
This is the code that starts a kind cluster that can be used to e2e
tests.
We are blocked by using the "Getting Started" scenario they provide
because it uses KubeadmControlPlane