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

Use the new re-entrant do-nothing script #6062

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

trexfeathers
Copy link
Contributor

@trexfeathers trexfeathers commented Jul 18, 2024

🚀 Pull Request

Description

Just a draft so we can trial it in the upcoming release.

https://github.com/SciTools-incubator/nothing

  • need to set appropriate permissions for the new repo

Consult Iris pull request check list


Add any of the below labels to trigger actions on this PR:

  • benchmark_this Request that this pull request be benchmarked to check if it introduces performance shifts

Copy link

codecov bot commented Jul 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.77%. Comparing base (b8f554f) to head (84818cf).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6062   +/-   ##
=======================================
  Coverage   89.77%   89.77%           
=======================================
  Files          88       88           
  Lines       23026    23026           
  Branches     5036     5036           
=======================================
  Hits        20672    20672           
  Misses       1623     1623           
  Partials      731      731           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@trexfeathers trexfeathers self-assigned this Jul 19, 2024
Comment on lines 732 to 733
f"{self.whats_news.latest.absolute()}\n"
f"{self.whats_news.template.absolute()}\n"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There will be Git commands that can get hold of these files.

Perhaps explicitly make the process:

  • Restore template
  • Create latest.rst from the template
  • Specifically mention bits that need changing/removing?

Copy link
Contributor Author

@trexfeathers trexfeathers Aug 12, 2024

Choose a reason for hiding this comment

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

Comment on lines 18 to 19
# pip install git+https://github.com/SciTools-incubator/nothing.git
from nothing import Progress
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would be better off as a try-except. If the import fails, provide instructions about WHY, then HOW to install nothing.

Copy link
Contributor Author

@trexfeathers trexfeathers Aug 12, 2024

Choose a reason for hiding this comment

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

Comment on lines 165 to 174
message = (
"Update the file ``etc/cf-standard-name-table.xml`` to the "
"latest CF standard names, via a new Pull Request.\n"
"(This is used during build to automatically generate the "
"sourcefile ``lib/iris/std_names.py``).\n"
"Latest standard names:\n"
'wget "https://cfconventions.org/Data/cf-standard-names'
'/current/src/cf-standard-name-table.xml";'
)
self.wait_for_done(message)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This wget command could easily put the new table into the correct place, too.

Copy link
Contributor Author

@trexfeathers trexfeathers Aug 12, 2024

Choose a reason for hiding this comment

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

Comment on lines +271 to +275
whatsnew_title = (
f"{self.strings.series} ({datetime.today().strftime('%d %b %Y')}"
)
if self.is_release_candidate:
whatsnew_title += " [release candidate]"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The title needs a bracket at the end

Copy link
Contributor Author

@trexfeathers trexfeathers Aug 12, 2024

Choose a reason for hiding this comment

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

Comment on lines 302 to 303
f"Review {self.whats_news.release.name} to ensure it is a good "
f"reflection of what is new in {self.strings.series}."
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Useful to have example(s) here. E.g. something big that we definitely want people to know about like a NumPy pin.

Copy link
Contributor Author

@trexfeathers trexfeathers Aug 12, 2024

Choose a reason for hiding this comment

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

Comment on lines 655 to 657
f"conda create -n tmp_iris{channel_command}iris="
f"{self.strings.release};\n"
f"conda remove -n tmp_iris --all;"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Include an import iris step.

Copy link
Contributor Author

@trexfeathers trexfeathers Aug 12, 2024

Choose a reason for hiding this comment

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

Comment on lines 677 to 680
"Update the release page in GitHub discussions, with the above "
"links "
"and anything else appropriate.\n"
"https://github.com/SciTools/iris/discussions"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Encourage copying from a previous example.

Copy link
Contributor Author

@trexfeathers trexfeathers Aug 12, 2024

Choose a reason for hiding this comment

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

"and anything else appropriate.\n"
"https://github.com/SciTools/iris/discussions"
)
self.wait_for_done(message)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another step? Comment on the discussion in case anyone is following it.

Copy link
Contributor Author

@trexfeathers trexfeathers Aug 12, 2024

Choose a reason for hiding this comment

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

message = (
"Announce the release via https://twitter.com/scitools_iris, "
"and any "
"other appropriate message boards (e.g. Yammer).\n"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
"other appropriate message boards (e.g. Yammer).\n"
"other appropriate message boards (e.g. Viva Engage).\n"

Copy link
Contributor Author

@trexfeathers trexfeathers Aug 12, 2024

Choose a reason for hiding this comment

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

tools/release_do_nothing.py Show resolved Hide resolved
_wait_for_done(message)
def validate(sha256_string: str) -> str:
try:
sha_len = int(sha256_string, 16).bit_length() + 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This doesn't actually work - the bit_length() of the string can be anywhere from 253 to 256. The correct two things to check are:

  • That the string can be validly parsed as a 16-bit number (int(sha256_string, 16))
  • That the string is 64 characters long

Copy link
Contributor Author

@trexfeathers trexfeathers Aug 14, 2024

Choose a reason for hiding this comment

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

"conda activate tmp_iris;\n"
f"pip install scitools-iris=={self.strings.release};\n"
'python -c "import iris; print(iris.__version__)";\n'
"conda remove -n tmp_iris --all;\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

This requires a "conda deactivate" to work

Copy link
Contributor Author

@trexfeathers trexfeathers Aug 14, 2024

Choose a reason for hiding this comment

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

f"git push -u origin {working_branch};"
"After the PR is merged, wait for the CI to complete, after which "
"the new version of Iris will be on conda-forge's servers.\n"
"https://github.com/conda-forge/iris-feedstock/actions"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
"https://github.com/conda-forge/iris-feedstock/actions"
"https://dev.azure.com/conda-forge/feedstock-builds/_build?definitionId=464"

Copy link
Contributor Author

@trexfeathers trexfeathers Aug 14, 2024

Choose a reason for hiding this comment

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

)
message = (
f"Comment on {discussion_url} to notify anyone watching that "
f"{self.strings.tag} has been released."
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
f"{self.strings.tag} has been released."
f"{self.git_tag} has been released."

Copy link
Contributor Author

@trexfeathers trexfeathers Aug 14, 2024

Choose a reason for hiding this comment

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

Comment on lines 617 to 625
message = (
"Follow the latest conda-forge guidance for creating a new "
"release candidate branch from the `main` branch:\n"
"https://conda-forge.org/docs/maintainer/knowledge_base.html#pre-release-builds\n\n"
"DEVIATION FROM GUIDANCE: config file(s) should point to "
"the `rc_iris` label (this is not the name that "
"conda-forge suggest).\n"
)
rc_branch = self.get_input(message, "Input the name of your new branch")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The changes are best made within the same PR that proposes the new release.

Copy link
Contributor Author

@trexfeathers trexfeathers Aug 14, 2024

Choose a reason for hiding this comment

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

@trexfeathers trexfeathers marked this pull request as ready for review August 14, 2024 14:53
@pp-mo
Copy link
Member

pp-mo commented Sep 12, 2024

Tested this in use + it was handy
But it would be nice if the "nothing" package allowed you to restart from latest stored.
I created a couple of issues to suggest improvements in the common code package : SciTools-incubator/nothing#1 SciTools-incubator/nothing#2

@trexfeathers
Copy link
Contributor Author

Tested this in use + it was handy But it would be nice if the "nothing" package allowed you to restart from latest stored. I created a couple of issues to suggest improvements in the common code package : SciTools-incubator/nothing#1 SciTools-incubator/nothing#2

Thanks @pp-mo these are great suggestions! I don't consider them a blocker to this PR, which I know @HGWright is looking at today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

4 participants