Remove gcr.io/k8s-skaffold repository from examples #3368
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
corneliusweig
requested review from
balopat,
briandealwis,
chanseokoh,
dgageot,
loosebazooka,
nkubala,
TadCordle and
tejal29
as code owners
Codecov Report
|
corneliusweig
force-pushed
the
w/examples-default-repo
branch
3 times, most recently
from
8bc58fd to
856e0b5
Compare
|
Not sure what's happening with the integration tests on travis, I'm looking... |
|
So, e.g.: |
balopat
previously requested changes
Dec 18, 2019
|
Sorry, I still didn't have time to look into this. I'll do this as soon as possible. Interesting though that kokoro is happy 🤔 |
Yeah only GCP_ONLY tests run on kokoro |
corneliusweig
force-pushed
the
w/examples-default-repo
branch
2 times, most recently
from
22747ef to
4f6807d
Compare
corneliusweig
commented
Dec 22, 2019
integration/skaffold/helper.go
Outdated
| @@ -262,6 +276,15 @@ func (b *RunBuilder) cmd(ctx context.Context) *exec.Cmd { | |||
| return cmd | |||
| } | |||
|
|
|||
| func isCoreCommand(command string) bool { | |||
| switch command { | |||
| case "build", "debug", "delete", "deploy", "dev", "generate-pipeline", "render", "run": | |||
There was a problem hiding this comment.
I'm not super-happy with this approach, as it mirrors what is already configured on the cobra command.
An alternative could be to create a NewSkaffoldCommand and filter valid arguments based on the cobra configuration. If this makes sense this should be done in a follow-up PR.
There was a problem hiding this comment.
How about using the cobra command to determine directly if the command has the given flags? Something like:
c := cmd.NewSkaffoldCommand(os.Stdout, os.Stderr)
var command *cobra.Command
for _, comm := range c.Commands() {
if comm.Name() == b.command {
command = comm
break
}
}
if b.ns != "" && command.Flags().Lookup("namespace") != nil {
args = append(args, "--namespace", b.ns)
}There was a problem hiding this comment.
Yes, this is what I was thinking too. Then I'll add it in this PR.
|
Finally the build is back to normal, sorry it took so long. I had trouble running the integration tests locally, as |
Signed-off-by: Cornelius Weig <[email protected]>
Signed-off-by: Cornelius Weig <[email protected]>
Signed-off-by: Cornelius Weig <[email protected]>
Skaffold deploy with default-repo seems to be not working that way.
Flags such as --namespace or --default-repo are applied to "all" Skaffold commands, but this only applies to all core commands. Make the test helper a bit smarter to not forward unknown arguments. Signed-off-by: Cornelius Weig <[email protected]>
…in integration tests Signed-off-by: Cornelius Weig <[email protected]>
When passing unknown flags to skaffold in integration tests, the tests rightfully fails. However, for simpler test setup it is desirable to unconditionally set some configurations. This change ensures that those flags will not break the integration tests. Signed-off-by: Cornelius Weig <[email protected]>
Signed-off-by: Cornelius Weig <[email protected]>
corneliusweig
force-pushed
the
w/examples-default-repo
branch
from
ab2a2d8 to
69ed501
Compare
|
It's rebased now and I also adapted the new buildpack example. Let's see if everything stays green :) |
dgageot
approved these changes
Jan 15, 2020
|
Oh dear.. there's another issue with a helm test. I need to take a look later today. |
|
Ok, I couldn't wait to fix that right now. I really hope this was the last issue. I think this is because the helm deployer does not correctly rewrite image names (possibly just for |
|
@corneliusweig bad news |
|
@dgageot 🤦♂️ well that one goes on me. Sorry for causing so much back-and-forth. Would you care for another try? |
kelsk
pushed a commit
to kelsk/skaffold
that referenced
this pull request
Apr 16, 2021
…ools#3368) * Always use the default repository `gcr.io/k8s-skaffold` in tests Signed-off-by: Cornelius Weig <[email protected]> * Remove gcr.io/k8s-skaffold repository from image names Signed-off-by: Cornelius Weig <[email protected]> * Convert former default-repo test to explicit repo tests Signed-off-by: Cornelius Weig <[email protected]> * Adapt image names in examples Signed-off-by: Cornelius Weig <[email protected]> * Revert helm integration test Skaffold deploy with default-repo seems to be not working that way. * Restrict flags to core commands in test-helper Flags such as --namespace or --default-repo are applied to "all" Skaffold commands, but this only applies to all core commands. Make the test helper a bit smarter to not forward unknown arguments. Signed-off-by: Cornelius Weig <[email protected]> * Adapt expectations to account for --default-repo=gcr.io/k8s-skaffold in integration tests Signed-off-by: Cornelius Weig <[email protected]> * Silently ignore unknown flags in integration tests When passing unknown flags to skaffold in integration tests, the tests rightfully fails. However, for simpler test setup it is desirable to unconditionally set some configurations. This change ensures that those flags will not break the integration tests. Signed-off-by: Cornelius Weig <[email protected]> * Use default repo in buildpack example Signed-off-by: Cornelius Weig <[email protected]> * Revert helm-test: use fully qualified image name * Change to fully qualified image in explicit repo integration test
This was referenced Jan 24, 2022
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.
Description
All examples reference images in the
gcr.io/k8s-skaffold/repository. The underlying reason for this is that Skaffold uses these examples in its integration tests, and pushes the built images to the registry.However, for regular Skaffold users the repository is not writable, so they need to modify the image names to try out the examples. The goal of this PR is to improve the usability of examples for Skaffold users. Removing the
gcr.io/k8s-skaffoldimage prefix will allow to run examples with just the--default-repooption and without having to modify theskaffold.yaml.User facing changes
Images in examples no longer reference the
gcr.io/k8s-skaffoldrepository.Before
Users could not use the
--default-repooption to push to their own registries.After
Users can use the
--default-repooption to push to their own registries.Next PRs.
n/a
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
Open question
Should the README to run examples include instructions for running with the
--default-repooption?Reviewer Notes