Skip to content
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

Rename sslStrict argument as sslInsecure #60

Merged

Conversation

afeblot
Copy link
Contributor

@afeblot afeblot commented May 21, 2019

Rename sslStrict to sslInscure in order to make the parameter more consistent with its actual behavior, to help with #58.

@carlowahlstedt
Copy link
Owner

This matches exactly what I was thinking from our discussion in the issue. Please set the version number back so a new major increment doesn’t happen. The publish of the task takes care of the versioning automatically.

@afeblot
Copy link
Contributor Author

afeblot commented May 21, 2019

Hi,
I will if you confirm you want to, but don't you think changing a parameter name is breaking the api and deserve a new major version? (these parameter names may not be visible in the GUI form of the extension, but they are the public api of the yaml way to use it)

@carlowahlstedt
Copy link
Owner

You make a good point and I think for that reason, it's not going to make sense to make this change. There isn't a good reason to break this and not make it backward compatible, even between major versions.

@afeblot
Copy link
Contributor Author

afeblot commented May 23, 2019

Oh, come on, don't be shy! That's what major version changes are for.
Microsoft is pushing hard to move to the yaml way, this will soon be the way to code a pipeline. All your users will probably be fooled by this variable name without even realizing it, the same way we were.
Fixing this will hurt no existing pipeline, and will make it right for your future users.

@carlowahlstedt
Copy link
Owner

@afeblot I'm on board with doing this. Just need to make sure that we include a section in the readme so people understand it's a breaking change. Otherwise, there will be a lot of upset and more importantly confused users. Would you mind to add something?

@carlowahlstedt
Copy link
Owner

Forgot the release is setup to auto-create release notes. This should help with this, but still need something in the readme.

@afeblot
Copy link
Contributor Author

afeblot commented Jul 1, 2019

Thanks @carlowahlstedt .
Readme updated as requested.

@carlowahlstedt carlowahlstedt merged commit 5378b2f into carlowahlstedt:master Jul 29, 2019
@carlowahlstedt
Copy link
Owner

Finally getting back to this. It all looks good. Thanks for the thought and work here. It will be helpful with yaml going forward as you mentioned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants