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

loadjson: do not send http_basic_authentication if not needed #1208

Merged
merged 1 commit into from Jul 11, 2022

Conversation

chaen
Copy link
Contributor

@chaen chaen commented Aug 30, 2021

I came across storages that return HTTP 400 if sent basic auth (S3). This PR only sends the username/password parameters if needed.

@chaen chaen requested a review from a team as a code owner August 30, 2021 15:00
@CLAassistant
Copy link

CLAassistant commented Aug 30, 2021

CLA assistant check
All committers have signed the CLA.

@puppet-community-rangefinder
Copy link

loadjson is a function

Breaking changes to this file MAY impact these 1 modules (near match):

This module is declared in 318 of 578 indexed public Puppetfiles.


These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

@carabasdaniel
Copy link
Contributor

Hi @chaen, this change looks good, however there are still a couple of minor syntax issues that need to be addressed.
We would really appreciate if you can put up a test in loadjson_spec to highlight this issue so that we can avoid possible regressions in the future.
Thank you for your contribution.

@chaen
Copy link
Contributor Author

chaen commented Oct 6, 2021

Hi @carabasdaniel sorry for the delay.
This is the first time I have to play with ruby/rake/etc so apologies if this takes a few iterations :-)
So I modified the tests to reflect the changes.
The various cases (no identity, user only, user + password) were already tested.
However, what is not covered is fetching a json from an S3 storage (which is what triggered this PR to start with).
These seems to be unit test, so I don't think I should add an actual S3 query there. Would there be a place for such tests ?

@david22swan
Copy link
Member

@chaen Look's like we're getting a few failures in the spec tests i you could give them a closer look

@chaen
Copy link
Contributor Author

chaen commented Nov 22, 2021

Hi @david22swan
these logs are all very cryptic to me but I have the impression that the windows failure is due to a CI problem more than the changes I did.

For the other error (puppet 6.0), I am happy to try and correct, but I have a very hard time reproducing (puppetlabs/iac#317 (comment)). Any help in that direction is more than welcome !

@chelnak
Copy link
Contributor

chelnak commented Feb 11, 2022

I've rebased PR on to main so we have got the latest changes. It will also let us see the logs of the failing task.

@chaen is this still something that you would like to work on?

@chaen
Copy link
Contributor Author

chaen commented Feb 11, 2022

Thanks @chelnak I am still interested in making it work, but I don't have much time to dedicate to it. So I can give it another go, hoping I don't bump into puppetlabs/iac#317 (comment) anymore

@chelnak
Copy link
Contributor

chelnak commented Feb 11, 2022

Excellent! Thank you for your response. Let's see what happens & try to get this one resolved.

@github-actions
Copy link

This PR has been marked as stale because it has been open for a while and has had no recent activity. If this PR is still important to you please drop a comment below and we will add this to our backlog to complete. Otherwise, it will be closed in 7 days.

@github-actions github-actions bot added the stale label Apr 26, 2022
@github-actions github-actions bot closed this May 3, 2022
@chelnak chelnak reopened this May 3, 2022
@puppet-community-rangefinder
Copy link

loadjson is a function

Breaking changes to this file MAY impact these 1 modules (near match):

This module is declared in 318 of 579 indexed public Puppetfiles.


These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

@github-actions github-actions bot closed this May 11, 2022
@LukasAud LukasAud reopened this May 11, 2022
@puppet-community-rangefinder
Copy link

loadjson is a function

Breaking changes to this file MAY impact these 1 modules (near match):

This module is declared in 318 of 579 indexed public Puppetfiles.


These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

@LukasAud LukasAud removed the stale label May 11, 2022
@github-actions
Copy link

Hello! 👋

This pull request has been open for a while and has had no recent activity. We've labelled it with attention-needed so that we can get a clear view of which PRs need our attention.

If you are waiting on a response from us we will try and address your comments on a future Community Day.

Alternatively, if it is no longer relevant to you please close the PR with a comment.

Please note that if a pull request receives no update for 7 after it has been labelled, it will be closed. We are always happy to re-open pull request if they have been closed in error.

@petergmurphy
Copy link
Contributor

Hi @chaen,

We've rebased your PR onto main. The spec tests that are failing are not directly related to your changes.

Your changes will now be merged.

Thanks for contributing! 🙂

@petergmurphy petergmurphy merged commit 16c27c8 into puppetlabs:main Jul 11, 2022
@chaen
Copy link
Contributor Author

chaen commented Jul 22, 2022

Hi @petergmurphy
Glad to see that, and sorry I did not get back to that PR sooner. But you know... life... 😃

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

Successfully merging this pull request may close these issues.

None yet

7 participants