Skip to content

Conversation

@valeriupredoi
Copy link
Contributor

@valeriupredoi valeriupredoi commented Jul 8, 2022

Description

Temporarily fixes test failures noted in #1655

Does not offer a long-term fix. That should be included after the 2.6.0 release.

Link to documentation:


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.

@codecov
Copy link

codecov bot commented Jul 8, 2022

Codecov Report

Merging #1659 (184049e) into main (aacec50) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1659   +/-   ##
=======================================
  Coverage   91.49%   91.49%           
=======================================
  Files         204      204           
  Lines       11175    11175           
=======================================
  Hits        10225    10225           
  Misses        950      950           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aacec50...184049e. Read the comment docs.

@valeriupredoi
Copy link
Contributor Author

valeriupredoi commented Jul 8, 2022

why is the doc build test trying to build the cf-units wheel? It's there in the already created env! What the...

nevermind, it looks like we should still exclude that post0 postrelease candidate

@valeriupredoi
Copy link
Contributor Author

ok tests pass - let's get this kludge in! 🍺

@sloosvel
Copy link
Contributor

That should be included after the 2.6.0 release.

Wouldn't that break the final release candidate if this was included after 2.6?

Copy link

@zklaus zklaus left a comment

Choose a reason for hiding this comment

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

Note that this must also be done in the conda-forge repo.

@zklaus
Copy link

zklaus commented Jul 11, 2022

That should be included after the 2.6.0 release.
Wouldn't that break the final release candidate if this was included after 2.6?

I am not sure I understand, but I think @valeriupredoi was referring to the long-term fix being put in place after the release, this short-term fix should go in asap. With this understanding, the sequence of events would be

  1. Merge this PR to make sure the nightly tests pass and we don't get any (additional) calendar problems in the 2.6.0 release
  2. Make any number of release candidates for ESMValCore
  3. Release the last release candidate the official 2.6.0 release for ESMValCore and release together with the 2.6.0 release of ESMValTool
  4. Put in the long-term fix. The proposal in Adapt tests to new version of cf-units #1656 works with both older and newer versions of cf-units.
  5. Profit

In this, there is no breaking of the release candidate. Does that make sense and answer your question?

@sloosvel
Copy link
Contributor

Sure, thanks. I understand now.

@sloosvel sloosvel merged commit 5217451 into main Jul 11, 2022
@sloosvel sloosvel deleted the pin_cf-units branch July 11, 2022 09:07
@valeriupredoi
Copy link
Contributor Author

as Klaus says, I especially like point 5. 😆 Cheers for approving and merging, guys! The change in conda feedstock should be done at 2.6.0rc-whatever the number (sorry lost track) - tests passed fine, so we're out of the woods for now, I have to rerun the conda lock test though

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

Labels

installation Installation problem testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants