Skip to content

Set some default retries for url.yml in the install task #279

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

Merged
merged 3 commits into from
Nov 15, 2024

Conversation

jajik
Copy link
Contributor

@jajik jajik commented Nov 5, 2024

It occurs pretty regularly (at least in my fork) that one of the downloads fails. This PR adds a few retries so that some random glitches are eliminated.

@jajik
Copy link
Contributor Author

jajik commented Nov 5, 2024

Also, is there any reason for running CI only on main branches for pushes? It seems rather inconvenient for the development..

@jajik
Copy link
Contributor Author

jajik commented Nov 5, 2024

Also, is there any reason for running CI only on main branches for pushes? It seems rather inconvenient for the development..

I've just created #280 if you'd like to address it.

@rpelisse
Copy link
Contributor

rpelisse commented Nov 6, 2024

Thanks, it's something I've been toying around in my head for while... LGTM, @guidograzioli any concerns?

@rpelisse rpelisse self-assigned this Nov 6, 2024
@rpelisse rpelisse added enhancement New feature or request minor_changes New parameters added to modules, or non-breaking behavior changes to existing parameters; no bugfix labels Nov 6, 2024
@rpelisse
Copy link
Contributor

rpelisse commented Nov 6, 2024

@jajik If you would not mind updating your PR to replace the hard coded value by variable defined in the roles' defaults, that would be great :) Depending on env, I expect users to want to tweak these settings.

@jajik
Copy link
Contributor Author

jajik commented Nov 6, 2024

@rpelisse I'm afraid it's getting a little bit overengineered, but check out the latest changes.

@rpelisse
Copy link
Contributor

rpelisse commented Nov 6, 2024

@jajik Oh sorry, that was maybe misleading. I did not meant adding Envvars, just variable in the defaults values of the role:

roles/jws/defaults.yml
...
jws_url_download_timeout: 
jws_url_download_retries: 

@jajik
Copy link
Contributor Author

jajik commented Nov 6, 2024

@rpelisse Check it out now.

@rpelisse
Copy link
Contributor

rpelisse commented Nov 6, 2024

That's great! And thanks you for adding the documentation, we often forget to do that.

Can I ask you for two more changes?

  1. We also need to document this new defaults in the meta/arguments_specs.yml file
  2. Can you remove the extra whitespaces added in roles/jws/README.md?

After that, I think we should be good to go (or rather good to rebase & merge)!

@jajik
Copy link
Contributor Author

jajik commented Nov 6, 2024

2. Can you remove the extra whitespaces added in roles/jws/README.md?

Which spaces do you mean? Feel free to mark them directly through a review. I tried to preserve the "nice" table structure.

@guidograzioli
Copy link
Member

These two:
jws_url_download_timeout:
jws_url_download_retries:

need to be defined in meta/argument_specs.yml too

@jajik
Copy link
Contributor Author

jajik commented Nov 7, 2024

These two: jws_url_download_timeout: jws_url_download_retries:

need to be defined in meta/argument_specs.yml too

Yes, @rpelisse mentioned it yesterday. I'll do that as soon as he'll get back wrt the whitespaces so that it's done at once. 🙂

@jajik
Copy link
Contributor Author

jajik commented Nov 11, 2024

2. Can you remove the extra whitespaces added in roles/jws/README.md?

Which spaces do you mean? Feel free to mark them directly through a review. I tried to preserve the "nice" table structure.

@rpelisse ^^

@rpelisse
Copy link
Contributor

@jajik Oh sorry, I thought it was clear! I just looked at the tabs "changes" of the PR and there you can see a lot of whitespaces have been added to the table:
whitespaces

@jajik
Copy link
Contributor Author

jajik commented Nov 11, 2024

@jajik Oh sorry, I thought it was clear! I just looked at the tabs "changes" of the PR and there you can see a lot of whitespaces have been added to the table: whitespaces

Yes, that's because the new variables are wider than the column, so in order to keep it aligned the same way it is now, it is necessary to add a few more.

If you don't want to have the columns aligned any longer, should I remove the extra spaces already in as well?

@rpelisse
Copy link
Contributor

Then, I would just separate this changes into a one commit with the comment "whitespaces", if you don't mind.

@jajik
Copy link
Contributor Author

jajik commented Nov 11, 2024

Then, I would just separate this changes into a one commit with the comment "whitespaces", if you don't mind.

so you want to preserve the formatting, but in a different commit?

@rpelisse
Copy link
Contributor

Yes, exactly - sorry if it was not clear. The rational is that this way we'll get a cleaner commit for the actual change, but also (even if it does not apply here), doing formatting in a separate commit is helping down the road if we use git bisect, for instance, to determine, which commit introduced a regression.

(Once, I was able to id like that the faulty commit, but it was littered by whitespaces changes that made it difficult to figure out what actual change could be the issue... I've learned my lesson ;) )

@jajik
Copy link
Contributor Author

jajik commented Nov 11, 2024

Ok, everything should be in place now.

@rpelisse rpelisse merged commit 79ede22 into ansible-middleware:main Nov 15, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request minor_changes New parameters added to modules, or non-breaking behavior changes to existing parameters; no bugfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants