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

Config Client: Fix logic to try all URLs #2763

Conversation

marnee01
Copy link
Contributor

There was a miss when implementing the MultipleUriStrategy.ALWAYS. It was not trying the next URL if response was null, or if the HTTP Status Code was not OK, but no exception was thrown.

(This author does not know under what scenario null would be returned as a response, but the code was already checking for it, so I updated the logic to also try the next URL for null response.)

Closes gh-2735

There was a miss when implementing the MultipleUriStrategy.ALWAYS.
It was not trying the next URL if response was null, or if
the HTTP Status Code was not OK, but no exception was thrown.

(This author does not know under what scenario null would be
returned as a response, but the code was already checking for it,
so I updated the logic to also try the next URL for null response.)

Closes spring-cloudgh-2735

Signed-off-by: Marnee DeRider <[email protected]>
@marnee01
Copy link
Contributor Author

In order to get the DCO check to complete, do I need to do something in my repo fork? I reread the instructions in the README about contributing and I didn't see any instructions regarding how to create a PR at all.

@ryanjbaxter
Copy link
Contributor

ryanjbaxter commented Mar 6, 2025

To fix the DCO check, you can follow the instructions here https://github.com/spring-cloud/spring-cloud-config/pull/2763/checks?check_run_id=37743759280

In the future you can sign your commits by adding -s when running git commit.

@marnee01
Copy link
Contributor Author

marnee01 commented Mar 6, 2025

@ryanjbaxter Thanks for the link. I had read the README and found the link about the DCO. It says it can be added automatically, but doesn't say it has to be. I had some trouble with it so I added it to the commit manually. The merge check says, "There is one commit incorrectly signed off. This means that the author of this commit failed to include a Signed-off-by line in the commit message." But the commit has the following:

Signed-off-by: Marnee DeRider [email protected]

I would like to ask what's wrong with it, but I guess I can't directly change it after the fact, so I'll have to recommit it or do the rebasing using the option to sign off automatically.

Unfortunately, it was surprisingly painful switching from my work account to this one on my computer (and then to switch). I flailed around until it worked, so I don't have a written set of notes with clear instructions for myself. It will be a few days before I will have time to address this. I'll close this now, since I will be resubmitting against the 4.1.x branch anyway.

@marnee01 marnee01 closed this Mar 6, 2025
@ryanjbaxter
Copy link
Contributor

Ok sorry it was a pain. Tag me on the new pr so I don't miss it

@marnee01
Copy link
Contributor Author

marnee01 commented Mar 6, 2025

Thanks @ryanjbaxter, will do. (It's just my lack of experience having had to switch before and some local restrictions.)

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.

Next URL is not attempted if not successful, but exception is not thrown
3 participants