✨ Add support for regexp in profile activation kubeContext #2065
Conversation
charlyx
requested review from
balopat,
dgageot,
nkubala,
priyawadhwa and
tejal29
as code owners
Codecov Report
@@ Coverage Diff @@
## master #2065 +/- ##
=========================================
+ Coverage 56.12% 56.3% +0.18%
=========================================
Files 180 180
Lines 7753 7801 +48
=========================================
+ Hits 4351 4392 +41
- Misses 2986 2991 +5
- Partials 416 418 +2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thanks for opening this - it's a good start!
Added some comments around ! prefix + clearer error messaging.
Also it would be great to update the docs page for profiles https://skaffold.dev/docs/how-tos/profiles/
pkg/skaffold/schema/profiles.go
Outdated
| @@ -124,7 +125,13 @@ func satisfies(expected, actual string) bool { | |||
| if strings.HasPrefix(expected, "!") { | |||
| return actual != expected[1:] | |||
There was a problem hiding this comment.
I think we should support the ! prefix for regular expressions too, e.g.:
!dev-.* should activate on all that don't match it
| matcher, err := re.Compile(expected) | ||
|
|
||
| if err != nil { | ||
| logrus.Infof("profile activation criteria '%s' is not a valid regexp, falling back to string", expected) |
There was a problem hiding this comment.
| logrus.Infof("profile activation criteria '%s' is not a valid regexp, falling back to string", expected) | |
| logrus.Infof("profile activation criteria '%s' is not a valid regexp, falling back to string: %v", expected, err) |
| @@ -377,8 +377,10 @@ func TestActivatedProfiles(t *testing.T) { | |||
| {Name: "activated", Activation: []latest.Activation{{KubeContext: "prod-context"}}}, | |||
| {Name: "not-activated", Activation: []latest.Activation{{KubeContext: "dev-context"}}}, | |||
| {Name: "also-activated", Activation: []latest.Activation{{KubeContext: "!dev-context"}}}, | |||
| {Name: "activated-regexp", Activation: []latest.Activation{{KubeContext: "prod-.*"}}}, | |||
| {Name: "not-activated-regexp", Activation: []latest.Activation{{KubeContext: "dev-.*"}}}, | |||
There was a problem hiding this comment.
we should cover the !(regex) and invalid regex cases too.
There was a problem hiding this comment.
Should I create another profile so I can test those cases more easily?
For instance, in order to test the invalid regex I would need a profile that matches when comparing strings, I guess. Also, is there any character limitation for the profile? I couldn't find any in create_context.go
Regarding the !regexp I'm not sure how to test it... Shall we assume that if the regexp is valid we're testing the regexp matching and not the string comparison so I can make a simple test?
There was a problem hiding this comment.
Feel free to add as many profiles as needed. Also, simple tests are better!
|
Hi @charlyx! Do you need help to finish this PR? |
|
Hi @dgageot 👋 Thanks for asking! |
|
Hi @charlyx, I'd like to see this PR merge. Anything I can do? |
|
@charlyx I'm going to merge this PR and add the tests |
dgageot
dismissed
balopat’s stale review
I'm going to add the tests for that PR in a separate PR
|
Sorry I couldn’t make it on time David.
I’ve been quite busy lately.
Thanks for the merge!
…Sent from my iPhone
On 5 Jul 2019, at 12:13 PM, David Gageot ***@***.***> wrote:
Merged #2065 into master.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Hi 👋
This PR adds support for regexp in profile activation via kubeContext as specified in #1677
I tried to avoid breaking any existing behaviour.
Thanks for your review 🙏
PS: thanks @pyaillet for his help 😄