-
Notifications
You must be signed in to change notification settings - Fork 345
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
add an option to always generate a docker image #253
Conversation
8876482
to
eaa9385
Compare
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.
Some comments inline..
@@ -25,6 +25,8 @@ import ( | |||
"path" | |||
"strings" | |||
|
|||
"github.com/scylladb/go-set/strset" |
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.
?
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.
bored to roll my own set collection :)
pkg/trait/deployment.go
Outdated
{ | ||
Key: "integration", | ||
Path: strings.TrimPrefix(s.Name, "/"), | ||
if d.ContainerImage == false { |
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 think a similar thing should be done for the Knative trait.
pkg/trait/deployment.go
Outdated
return true | ||
} | ||
|
||
if d.ContainerImage == true && |
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.
Maybe it makes more sense to move this code in a separate trait, there seems to be two distinct sections in this trait and the deployment trait is not active in all profiles.
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.
maybe we should add the deployment to all the profiles and differentiate according to the platform ? I can of course create a new trait but then we have to introduce dependencies among traits i.e. the deploy trait need to know if the integration image has to be created or not to determine if configmaps have to be included or not (and maybe something else)
eaa9385
to
a996d13
Compare
so I've added some logic in the deploy trait to skip creating resources when running on knative. btw, I'm wondering if the concept of profiles still matters or if each trait should be responsible about detecting what to do according to the environment. |
I have the same concerns around profiles, but I think the concept is still useful. I'm also working on the knative side of deployment. I'm going to open a PR. Sometimes the knative side should create a deployment unfortunately, so I've made the knative trait include a deployment trait by composition.. Let's discuss it in the PR. |
maybe a profile should act on the configuration of the traits instead of include/exclude them ? |
Well, sometimes you need to inspect metadata and cluster configuration to understand the flow you've to use. Their use to exclude completely a trait from the flow (e.g. exclude Knative in std deployment flow) is useful.. I just don't want to over-engineer them.. |
yup, don't know what's better. |
a996d13
to
596adc3
Compare
596adc3
to
c38482c
Compare
Fixes #246