URL quoting for matrix parameters

briantist commented 1 year ago

Fixes #408

Provides for quoting of matrix parameters to ensure their values (and names) end up in Artifactory correctly, but optionally.

actual implementation is in encode_matrix_parameters with a new required parameter quote_parameters (I consider this method not really part of the public API)
public-facing deploy method will take a parameter of the same name, to pass into the encode function
for now, the parameter has a default of None instead of False
- this is so that we can detect when no value was passed, and warn about the planned change of default in v0.10.0
- that change will require some minor implementation changes to be made, and I've added TODOs to help find those leading up to the change
unit tests added or changed for the new functionality
- test True, False, and None (ensure warnings; None tests should be removed when the default changes)
- some unit tests changed in order to avoid littering the test output with warnings (with TODOs to remove later)

Still left to do:

integration tests: I don't have a pro license key to use locally, so unfortunately I'll probably rely on CI for the integration runs

briantist commented 1 year ago

@allburov would appreciate you approving the CI run so I can work on the integration tests 🙏

briantist commented 1 year ago

I now realize the integration tests are not run in CI 🤦 I tried to run them locally but without a license key it seems impossible :( I'd rather not sign up for a trial right now...

I'm not sure how you want to proceed on that. I can make guesses on changes to the integration tests but it seems dangerous to do so.

Are you able to run integration and either make the changes in my branch, or show me the warnings/errors and I can try to make changes (this will probably only work if small changes are needed, our back-and-forth will be somewhat slow I think)

allburov commented 1 year ago

@briantist sorry, I don't have an Artifactory license too right now :(

@beliaev-maksim Could you help us with running integration tests for the PR?

beliaev-maksim commented 1 year ago

@allburov I also lost access to artifactory..

give me a couple of days. I will try to catch up with some people inside JFrog on the topic of license

briantist commented 1 year ago

Re: integration tests: of course I would prefer to have coverage in them, but I also don't believe they will be failing right now. I think they will simply show warnings until v0.10.0.

If we can get a license that would be great, but if not, I don't think I should modify the tests without someone being able to run them; greater chance that I will break something than help.

briantist commented 1 year ago

Hi @allburov & @beliaev-maksim just wanted to check in on this

beliaev-maksim commented 1 year ago

@allburov we might have an instance. I got some contacts. However, this will take time. So, please decide based on the urgency vs risk

allburov commented 1 year ago

It affects the actions too https://github.com/devopshq/artifactory/issues/419

kohodono commented 1 year ago

@allburov checked on 7.31.13 - it works!

briantist commented 1 year ago

@kohodono @allburov thank you!

briantist commented 1 year ago

@allburov do you have an idea when 0.9.0 might be released?

allburov commented 1 year ago

@briantist hi! I'm releasing it on the next Tuesday, 27 June Thanks for the help!

devopshq / artifactory

URL quoting for matrix parameters #409