Add support for setting default-repo in global config #1057
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1057 +/- ##
==========================================
+ Coverage 42.18% 42.29% +0.11%
==========================================
Files 89 90 +1
Lines 3999 4071 +72
==========================================
+ Hits 1687 1722 +35
- Misses 2143 2179 +36
- Partials 169 170 +1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
I tried out using a context, this is going to be awesome!
However I saw two important UX issues:
- setting the default_repo on a global level doesn't work (+ there is no "resolution" logic yet for when it is defined for a context as well globally)
- no way to "unset" the default repo
|
Another couple of thoughts:
|
nkubala
force-pushed
the
default-repo
branch
2 times, most recently
from
56b26d9 to
8fe8208
Compare
|
@balopat thanks for the feedback!
Can you elaborate? It seems to work for me.
I'll fix this in a separate PR.
This part is true, I was weighing options on the best approach for this. I think the right answer here for order of precedence is:
WDYT? RE: naming, I'm also of the mind that we should considering renaming the |
nkubala
force-pushed
the
default-repo
branch
2 times, most recently
from
d5ebebc to
0ef6535
Compare
…fix kubectx resolution in config list
nkubala
dismissed
balopat’s stale review
I think I addressed all your feedback
There was a problem hiding this comment.
this is very close, and I love using it!
I have some small nits + missing docs.
This is a really cool feature, I'd like to have it represented in our docs, but at least in all the examples:
skaffold dev --default-repo myrepo
# or
SKAFFOLD_DEFAULT_REPO=myrepo skaffold dev
| // prefixes don't match, concatenate and truncate | ||
| return truncate(defaultRepo + "/" + originalImage) | ||
| } | ||
| // TODO: check defaultRepo is < 256 chars when setting |
| @@ -42,7 +42,7 @@ type Tester interface { | |||
| // FullTester should always be the ONLY implementation of the Tester interface; | |||
| // newly added testing implementations should implement the Runner interface. | |||
| type FullTester struct { | |||
| testCases *[]latest.TestCase | |||
| testCases *[]*latest.TestCase | |||
There was a problem hiding this comment.
nit: this change shouldn't belong to this PR
| @@ -77,7 +77,7 @@ func (t FullTester) Test(ctx context.Context, out io.Writer, bRes []build.Artifa | |||
| return nil | |||
| } | |||
|
|
|||
| func (t FullTester) runStructureTests(ctx context.Context, out io.Writer, bRes []build.Artifact, testCase latest.TestCase) error { | |||
| func (t FullTester) runStructureTests(ctx context.Context, out io.Writer, bRes []build.Artifact, testCase *latest.TestCase) error { | |||
There was a problem hiding this comment.
nit: this change shouldn't belong to this PR
| @@ -32,7 +32,7 @@ import ( | |||
| // NewTester parses the provided test cases from the Skaffold config, | |||
| // and returns a Tester instance with all the necessary test runners | |||
| // to run all specified tests. | |||
| func NewTester(testCases *[]latest.TestCase) (Tester, error) { | |||
| func NewTester(testCases *[]*latest.TestCase) (Tester, error) { | |||
There was a problem hiding this comment.
nit: this change shouldn't belong to this PR
| @@ -55,7 +55,7 @@ func applyProfile(config *latest.SkaffoldPipeline, profile latest.Profile) { | |||
| Kind: config.Kind, | |||
| Build: overlayProfileField(config.Build, profile.Build).(latest.BuildConfig), | |||
| Deploy: overlayProfileField(config.Deploy, profile.Deploy).(latest.DeployConfig), | |||
| Test: overlayProfileField(config.Test, profile.Test).([]latest.TestCase), | |||
| Test: overlayProfileField(config.Test, profile.Test).([]*latest.TestCase), | |||
There was a problem hiding this comment.
nit: this change shouldn't belong to this PR
| @@ -237,7 +237,7 @@ type Artifact struct { | |||
| type Profile struct { | |||
| Name string `yaml:"name,omitempty"` | |||
| Build BuildConfig `yaml:"build,omitempty"` | |||
| Test []TestCase `yaml:"test,omitempty"` | |||
| Test []*TestCase `yaml:"test,omitempty"` | |||
There was a problem hiding this comment.
nit: this change shouldn't belong to this PR
| @@ -330,7 +330,7 @@ func TestRun(t *testing.T) { | |||
| }, | |||
| }, | |||
| }, | |||
| Test: []latest.TestCase{ | |||
| Test: []*latest.TestCase{ | |||
There was a problem hiding this comment.
nit: this change shouldn't belong to this PR
| @@ -140,16 +145,16 @@ func getBuilder(cfg *latest.BuildConfig, kubeContext string) (build.Builder, err | |||
| } | |||
| } | |||
|
|
|||
| func getTester(cfg *[]latest.TestCase) (test.Tester, error) { | |||
| func getTester(cfg *[]*latest.TestCase) (test.Tester, error) { | |||
There was a problem hiding this comment.
nit: this change shouldn't belong to this PR
| newImageName := util.SubstituteDefaultRepoIntoImage(h.defaultRepo, imageName) | ||
| build, ok := imageToBuildResult[newImageName] | ||
| if !ok { | ||
| return nil, fmt.Errorf("No build present for %s", imageName) |
There was a problem hiding this comment.
| return nil, fmt.Errorf("No build present for %s", imageName) | |
| return nil, fmt.Errorf("no build present for %s", imageName) |
nit: errors shouldn't start capitalized https://github.com/golang/go/wiki/CodeReviewComments#error-strings
| func ReadConfigForFile(filename string) (*Config, error) { | ||
| fmt.Printf("reading config for file %s\n", filename) |
There was a problem hiding this comment.
ux-nit: this feels like a bit of clutter on the screen - can we make it available only for debug logging?
|
I'll open a separate PR to address my nits, as I can't add to your branch. |
This PR enables users to set the "default-repo" value in the skaffold
global config. This value will be substituted into all provided images
in the skaffold.yaml and k8s manifests according to certain
heuristics, allowing a better user experience for developing
applications against different image registries.