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

Adapt to the new xclim-testdata structure #467

Merged
merged 2 commits into from
Aug 23, 2024
Merged

Conversation

Zeitsperre
Copy link
Contributor

Overview

This PR updates the cloning of the xclim-testdata repo to reflect structural changes.

Changes

Non-breaking changes

  • Adjusts the location of the xclim-testdata data folder

Related Issue / Discussion

CI Operations

birdhouse_daccs_configs_branch: master
birdhouse_skip_ci: false

@github-actions github-actions bot added the ci/deployment Related to deployment utilities and scripts label Aug 22, 2024
@Zeitsperre
Copy link
Contributor Author

@tlvu
We are currently grabbing main here, but the testdata is tagged according to version (which is based on the date). If we want, we could base this on the tags for less breakage potential.

The last stable tag was v2023.12.14.

@tlvu
Copy link
Collaborator

tlvu commented Aug 22, 2024

@tlvu We are currently grabbing main here, but the testdata is tagged according to version (which is based on the date). If we want, we could base this on the tags for less breakage potential.

The last stable tag was v2023.12.14.

@Zeitsperre

FYI, if you want to specify the tag v2023.12.14 you can do it here (replace origin/main with v2023.12.14)

So are we planning to only hardcode the tag just as a temporary transition measure or are we planning to go this route forward for production deployment?

@Zeitsperre
Copy link
Contributor Author

Once my PR at xclim-testdata is approved, I was going to tag a new commit. I don't see why we wouldn't want to use the tags going forward, though. xclim-testdata doesn't move very often, but this could prevent breakage for users.

@tlvu
Copy link
Collaborator

tlvu commented Aug 23, 2024

Once my PR at xclim-testdata is approved, I was going to tag a new commit.

Oh I didn't realized that you will want to synchronize the merge of this PR with your PR so the tagging is not really required.

I don't see why we wouldn't want to use the tags going forward, though. xclim-testdata doesn't move very often, but this could prevent breakage for users.

Hardcoded tags means:

  • need to update here each time for new tag
  • any quick change (fix existing data, add new data) will also need a tag to trigger the deploy
  • changing tag is slower to autodeploy (nightly) instead of the content (every 30 mins)

Since breaking change is not often, I think this is the first time, is it worth it? It's up to you.

@tlvu
Copy link
Collaborator

tlvu commented Aug 23, 2024

Since breaking change is not often, I think this is the first time, is it worth it? It's up to you.

Sorry to clarify, we can go the temporary route just for this time, so you do not need to worry about synchronizing merge.

My concerns were about always hardcoding going forward.

@Zeitsperre
Copy link
Contributor Author

@tlvu I follow your logic. It makes sense here. Given that the structure shouldn't change after this, as well as the fact that we uniquely glob NetCDF files, I don't think we need to use tags here.

Will finalize changes on the xclim PR, merge xclim-testdata, then finally merge the remaining PRs. I don't anticipate needing to make any other changes here after this.

@tlvu
Copy link
Collaborator

tlvu commented Aug 23, 2024

Oh by the way, you'll need to update the CHANGES.md at the root of the repo as well. Just quickly refer to your xclim-testdata PR for more info.

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Aug 23, 2024
@Zeitsperre Zeitsperre merged commit 7ae446a into master Aug 23, 2024
3 of 4 checks passed
@Zeitsperre Zeitsperre deleted the adapt-to-new-testdata branch August 23, 2024 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/deployment Related to deployment utilities and scripts documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants