Don't use is_secret attribute for secrets in variable groups

[AlexPykavy]

Because specifying the secret_value attribute is enough to designate the variable as secret.

All Submissions:

• Have you added an explanation of what your changes do and why you'd like us to include them?
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.
• My code follows the code style of this project.
• I ran lint checks locally prior to submission.
• Have you checked to ensure there aren't other open PRs for the same update/change?

What about the current behavior has changed?

Issue Number:

Does this introduce a change to go.mod, go.sum or vendor/?

• Yes
• No

Does this introduce a breaking change?

• Yes
• No

Any relevant logs, error output, etc?

(If it’s long, please paste to https://ghostbin.com/ and insert the link here.)

Other information

[xuzhang3]

@AlexPykavy Thank you for opening this PR. Is there any specific reason why to remove the is_secret property?

[AlexPykavy]

Because having the value, is_secret and secret_value triplet is redundant and confusing. If you want to define the secret, you need to first set two attributes: is_secret and secret_value.

For me, the more logical approach is to use binary logic: either value, or secret_value.

[xuzhang3]

You are right, value or secret_value is enough here. We originally introduced is_secret into the ADO provider for mapping to API parameters.

[HerrSubset]

This would be a great change. I just wasted waaay too much time trying to figure out why some secret variable was empty when using value = xxx in combination with is_secret = true.

Sure, it's documented and examples are available, but when you have worked with this provider only a couple of times, you might remember the is_secret property, but not necessarily that you also need to switch to secret_value. And then it gets confusing quickly...

Instead of removing the is_secret property, you could also remove secret_value, then the change doesn't have to be breaking immediately. For regular variables, users keep using value. For a secret variable, you can use either value or secret_value in combination with is_secret. But if you use secret_value, you get a warning, saying it's deprecated and you should use value instead?

Then after some time, you can remove secret_value in a breaking change. But at least there was a transition period.

Or another option: display an error when is_secret = true is defined without a secret_value.

[AlexPykavy]

Hi @HerrSubset , thank you for the upvoting. I've updated this PR to make it a little clearer. To be honest, even I had a hard time understanding some things since it was opened a long time ago 😆

I'll keep this one as it seems as a proper solution to the problem. At the same time, I agree that adding simple validation will help users of this provider right now, so I'll open a new one.