Improve support for custom dev environments #349
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Build Failed 😱 Build Id: 558780d5-cdd1-4235-8d81-b57ad7902006 Build Logs |
|
Build Failed 😱 Build Id: c05a02fe-5cf7-465f-b4a1-46170c4e5caa Build Logs |
|
Build Failed 😱 Build Id: 61ff666d-6805-40de-a72e-218828c167dd Build Logs |
|
Build Failed 😱 Build Id: 57d5d891-3bcc-43fb-a322-af542c212333 Build Logs |
|
Build Succeeded 👏 Build Id: 21987e30-2cd6-40a7-8ae1-6c7c1a5c8805 The following development artifacts have been built, and will exist for the next 30 days:
(experimental) To install this version:
|
victor-prodan
force-pushed
the
customcluster
branch
from
cb9ce4a
to
8738e0a
Compare
|
Build Succeeded 👏 Build Id: 18374f87-40fa-4748-966b-f08c00ca1678 The following development artifacts have been built, and will exist for the next 30 days:
(experimental) To install this version:
|
markmandel
reviewed
Sep 13, 2018
build/Makefile
Outdated
| # | |___| |_| \__ \ || (_) | | | | | | | ||
| # \_____\__,_|___/\__\___/|_| |_| |_| | ||
|
|
||
| setup-custom-test-cluster: $(ensure-build-image) |
There was a problem hiding this comment.
Minor nit: for consistency sake, this should likely be called custom-test-cluster (custom as the prefix)
Looking back, we should be consistent with the prefixes for cluster operations - gcloud for GKE (or even gke), minikube for minikube, etc. Which we mostly are.
There was a problem hiding this comment.
I named it 'setup' on purpose because, opposed to the gcloud-test-cluster or 'minikube-test-cluster' it's not creating the cluster, but only preparing it for agones.
WDYT?
There was a problem hiding this comment.
you could make that target common to every provider and rename to reflect that and use it as a post-target in gcloud-test-cluster and minikube-test-cluster
markmandel
added
kind/documentation
|
@victor-prodan just checking in :) I assume you're busy on #340 - just wanted to make sure you weren't waiting on anything from me. @Kuqd Did you get a chance to have a look - wanted to make sure I didn't miss anything. |
|
I was actually waiting for some feedback from @Kuqd. He was suspiciouly silent lately 🤔 |
|
I will have look, sorry guys ! |
cyriltovena
suggested changes
Sep 18, 2018
There was a problem hiding this comment.
Long time ago we talked about splitting the makefile into multiple purpose makefile include. Is this the right moment ? Is it too much too ask within this review ? May be we could log an issue for it ? @markmandel
build/Makefile
Outdated
| # | |___| |_| \__ \ || (_) | | | | | | | ||
| # \_____\__,_|___/\__\___/|_| |_| |_| | ||
|
|
||
| setup-custom-test-cluster: $(ensure-build-image) |
There was a problem hiding this comment.
you could make that target common to every provider and rename to reflect that and use it as a post-target in gcloud-test-cluster and minikube-test-cluster
build/Makefile
Outdated
| $(DOCKER_RUN) kubectl apply -f $(mount_path)/build/helm.yaml | ||
| $(DOCKER_RUN) helm init --service-account helm | ||
|
|
||
| clean-custom-test-cluster: $(ensure-build-image) $(uninstall) |
There was a problem hiding this comment.
same here this could be common to every provider
|
@Kuqd: what do you mean by "splitting the makefile into multiple purpose makefile include" more exactly? I don't know... 😞 |
victor-prodan
force-pushed
the
customcluster
branch
from
8738e0a
to
a685d96
Compare
|
Build Succeeded 👏 Build Id: a697a514-e0f6-4dc6-b4d1-721bbb2abf76 The following development artifacts have been built, and will exist for the next 30 days:
(experimental) To install this version:
|
|
I hope I haven't forgotten any point.... |
|
Build Failed 😱 Build Id: 79bdfc12-ce68-4d47-91bb-7cab3a09445c Build Logs |
|
i think that was my fault, I was futzing with the cluster. |
|
@victor-prodan I might having multiple makefile one for build, one minikube, one for setup, etc... , using includes. But let's see that later, we should do another PR it's easier. |
markmandel
requested changes
Sep 20, 2018
markmandel
reviewed
Sep 20, 2018
victor-prodan
force-pushed
the
customcluster
branch
from
48b0aa4
to
e686eae
Compare
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
victor-prodan
force-pushed
the
customcluster
branch
from
e686eae
to
8fdc30e
Compare
|
CLAs look good, thanks! |
|
Build Succeeded 👏 Build Id: 7dcfbdd0-3d41-4f13-aebc-225c7e6dec8a The following development artifacts have been built, and will exist for the next 30 days:
(experimental) To install this version:
|
|
Build Succeeded 👏 Build Id: b29caff1-0156-4978-afea-d2ad6c52185d The following development artifacts have been built, and will exist for the next 30 days:
(experimental) To install this version:
|
|
Build Failed 😱 Build Id: 09fd97cb-1c13-4553-8da3-ce61e537fb7c Build Logs |
|
You possibly hit #357 ? @markmandel do you know why would be out of index |
|
@Kuqd : yes it's Mark's Bug 😉.Status.Ports is Nil... |
|
Yep, it's a race condition. Review and merge #357 and it will go away 👍 |
victor-prodan
force-pushed
the
customcluster
branch
from
8fdc30e
to
42d5219
Compare
|
Build Succeeded 👏 Build Id: 48744560-bb26-4c68-9532-a8588e4caab2 The following development artifacts have been built, and will exist for the next 30 days:
(experimental) To install this version:
|
cyriltovena
approved these changes
Sep 20, 2018
markmandel
approved these changes
Sep 21, 2018
This was referenced Sep 21, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Issue #348