-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Autoconfigure cloud if kubernetes url is not set #208
Conversation
Do it explicitly because kubernetes-client will change behavior Autoconfigure the kubernetes client for tests Use the currently enabled context
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.
Looks like a really useful change.
I am a bit worried that someone will just update the client dependency and therefore introduce a breaking change, though.
@@ -1,7 +1,7 @@ | |||
pipeline { | |||
agent { | |||
kubernetes { | |||
cloud 'kubernetes-plugin-test' | |||
// cloud 'kubernetes-plugin-test' |
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.
Not needed - remove?
@@ -1,7 +1,7 @@ | |||
pipeline { | |||
agent { | |||
kubernetes { | |||
//cloud 'kubernetes-plugin-test' | |||
//cloud 'kubernetes' |
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 guess that comment was changed by an automated IDE refactoring.
What was that comment for in the first place? Can we remove 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.
no, it's just that now the cloud is the default kubernetes
one
// although this will still autoconfigure based on Config constructor notes | ||
// In future releases (2.4.x) the public constructor will be empty. | ||
// The current functionality will be provided by autoConfigure(). | ||
// This is a necessary change to allow us distinguish between auto configured values and builder values. |
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 have to admit that I don't fully understand this comment.
I guess it means that by 2.4.x, this
new ConfigBuilder().withMasterUrl(serviceAddress)
Will behave differently, i.e. if we move the dependency to 2.4.x we will either introduce a breaking change, or we will have to change that line again?
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.
//In future releases (2.4.x) the public constructor will be empty.
//The current functionality will be provided by autoConfigure().
//This is a necessary change to allow us distinguish between auto configured values and builder values.
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.
But this will still cause a breaking change if somebody changes the client library to 2.4 and doesn't update this code here, right?
- before 2.4.x, providing a master url means: do autoconfigure for everything but the master url
- after 2.4.x, providing a master url will mean "do not autoconfigure at all".
Is that intended?
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.
yes, it may be a breaking change to document. Better than the current behavior because your jenkins settings may be ignored
btw client is in 2.6.x and code still there
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.
Better than the current behavior because your jenkins settings may be ignored
Right, I remember we had that problem as well.
The only remaining downside that remains is that as of now, you can't have autoconfigure AND overwrite the kubernetes URL, like it used to work.
One solution for this would be to add a checkbox for autoconfigure to make it explicit...
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 guess you should remove the client-side validation for serverUrl
:
kubernetes-plugin/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud.java
Lines 475 to 476 in 40d452f
if (StringUtils.isBlank(serverUrl)) | |
return FormValidation.error("URL is required"); |
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 this is good for merge except for missing documentation of the breaking change and that leaving the URL empty means "autoconfigure", i.e. in README.md
and and in help-serverUrl.html
.
Add permissions to get secrets
@carlossg one quick question please..
|
@vamsi248 you were told that questions must be asked in jenkins-user mailing list. I will not answer questions in the issue tracker no matter how many comments you leave |
Sry @carlosg . I send email. in that they will send reply as email only.. |
Do it explicitly because kubernetes-client will change behavior
Autoconfigure the kubernetes client for tests
Use the currently enabled context