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

Simplify configure task in recipe test workflow #3394

Merged
merged 7 commits into from
Oct 26, 2023

Conversation

alistairsellar
Copy link
Contributor

@alistairsellar alistairsellar commented Oct 25, 2023

Description

At present, the configure task in RTW reads the example user config file from the repo and overwrites with any variables provided by the workflow config (mainly DRS). However, ESMValTool doesn't require a user config file to contain all the keys specified in the user file, and will provide defaults for any keys not in the file (indeed the example file doesn't include all possible config variables). It therefore will be simpler to write only the keys provided by the RTW.

Benefits:

  • This simplifies the workflow by removing the need to obtain/read the example user config file, and
  • means that all the other keys default to values set by the code, rather than a mix of code and example file as at present

Before you get started

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.


To help with the number of pull requests:

@alistairsellar alistairsellar added the Recipe Test Workflow (RTW) Items relevant to the Recipe Test Workflow label Oct 25, 2023
@alistairsellar alistairsellar self-assigned this Oct 25, 2023
@alistairsellar alistairsellar marked this pull request as draft October 25, 2023 14:15
@alistairsellar
Copy link
Contributor Author

Changes to the dependencies result in the following graph, since configure no longer depends on clone

image

@valeriupredoi
Copy link
Contributor

nice one @alistairsellar - one thing I am very puzzled about is why I am being emailed about this by Github, since I am neither assigned nor the Tech list was invoked ❓

@alistairsellar
Copy link
Contributor Author

nice one @alistairsellar - one thing I am very puzzled about is why I am being emailed about this by Github, since I am neither assigned nor the Tech list was invoked ❓

Hrrm, curious. Are you subscribed to updates relating to recipe test workflow label? Is that even possible in GitHub? Or perhaps you've subscribed to all updates by me!? Lucky you!

@valeriupredoi
Copy link
Contributor

perhaps you've subscribed to all updates by me!?

oh noo, what did I do to me life? 🤣

Prob subscribed to RTW updates, gonna check 👍

@alistairsellar alistairsellar marked this pull request as ready for review October 26, 2023 07:38
@alistairsellar
Copy link
Contributor Author

HI @ehogan please can you review?

Docs test fails due to this warning:

WARNING: The default value for navigation_with_keys will change to False in the next release. If you wish to preserve the old behavior for your site, set navigation_with_keys=True in the html_theme_options dict in your conf.py file.Be aware that navigation_with_keys = True has negative accessibility implications:pydata/pydata-sphinx-theme#1492

Pretty sure this is unrelated to my changes, and hopefully will be solved by merging in changes from main. Not sure if I can/should merge into this branch, or better to merge this PR into feature branch and then merge main into feature branch?

@ehogan
Copy link
Contributor

ehogan commented Oct 26, 2023

Hi @alistairsellar, there is an issue open about the failing doc build (see #3395) so we can ignore this for now.

Once it is fixed, it is better to merge main into recipe_test_workflow_prototype then update this branch by pressing the button that pops up when this branch is out of date with the branch you are merging into (i.e. recipe_test_workflow_prototype). Does that make sense?

P.S. Would it be possible for you to create an issue for this PR, see "How we are working" on #2786 😊

@alistairsellar
Copy link
Contributor Author

Thanks @ehogan, yes merge sequence makes sense.

Issue #3398 opened.

Copy link
Contributor

@ehogan ehogan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @alistairsellar! 🥳

Would it be possible for you to update the documentation related to the configure task in recipe_test_workflow/doc/source/user_guide/workflow.rst, please? 😊

Note the changes I suggested to the graph produce the same visualisation as you posted:
graph

@alistairsellar
Copy link
Contributor Author

Would it be possible for you to update the documentation related to the configure task in recipe_test_workflow/doc/source/user_guide/workflow.rst, please? 😊

Thanks @ehogan, changes made. In the absence of working readthedocs test, I checked that the updated RTW documentation builds on my machine.

@ehogan
Copy link
Contributor

ehogan commented Oct 26, 2023

Would it be possible for you to update the documentation related to the configure task in recipe_test_workflow/doc/source/user_guide/workflow.rst, please? 😊

Thanks @ehogan, changes made. In the absence of working readthedocs test, I checked that the updated RTW documentation builds on my machine.

Because the RTW documentation is currently seperate from the main documentation, we do have checks for them; click Details on one of the rtw-tests checks to see :)

Copy link
Contributor

@ehogan ehogan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, thanks @alistairsellar! 🥳

@ehogan
Copy link
Contributor

ehogan commented Oct 26, 2023

There is no need to wait until the failing doc build is fixed to merge this, so please go ahead and merge @alistairsellar! 😊

@ehogan ehogan linked an issue Oct 26, 2023 that may be closed by this pull request
@alistairsellar alistairsellar merged commit 58b5486 into recipe_test_workflow_prototype Oct 26, 2023
7 of 8 checks passed
@alistairsellar alistairsellar deleted the rtw_simplify_configure branch October 26, 2023 09:57
@alistairsellar
Copy link
Contributor Author

Thanks @ehogan !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Recipe Test Workflow (RTW) Items relevant to the Recipe Test Workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simplify configure task in recipe test workflow
3 participants