-
Notifications
You must be signed in to change notification settings - Fork 344
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 unit tests for the classpath trait #255 #846
Conversation
assert.NotNil(t, environment.Classpath) | ||
assert.Equal(t, strset.New("/etc/camel/resources", "./resources"), environment.Classpath) | ||
|
||
assert.Len(t, container.Env, 0) |
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 wonder if we shouldn't expect a length of 1 here ? It would then possibly be a bug where modification happens in a copy of the deployment because of Visit() pass value by copy.
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 the classpath should be set by the container
trait instead of the classpath
one which should be in charge only to compute Environment.Classpath
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.
@aldettinger do you have bandwidth to wotk on it ?
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.
Hey @lburgazzoli, thanks for review.
I don't have that much of bandwidth and would prefer focusing on completing tests for other traits in order to close #255.
Would it make sense that I merge this PR and create a separate issue to move the classpath setup in the contrainer trait ?
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.
goign to merge and I will work on the container trait if you don't mind
assert.NotNil(t, environment.Classpath) | ||
assert.Equal(t, strset.New("/etc/camel/resources", "./resources", "/mount/path"), environment.Classpath) | ||
|
||
assert.Len(t, container.Env, 0) |
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.
Same here, shouldn't we expect a length of 1 ?
@@ -87,7 +87,7 @@ func (t *classpathTrait) Apply(e *Environment) error { | |||
e.Classpath.Add(artifact.Target) | |||
} | |||
|
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 it makes sense, but let me know
My first go PR ever, please double-check :)
My dev environment is not fully working, so I've just run make lint and make test.