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

Introducing release script #390

Merged
merged 3 commits into from May 27, 2020
Merged

Introducing release script #390

merged 3 commits into from May 27, 2020

Conversation

fao89
Copy link
Member

@fao89 fao89 commented May 8, 2020

Introducing release script

which executes the first 6 steps of https://pulp.plan.io/projects/pulp/wiki/Pulp3_Release_Guide

https://pulp.plan.io/issues/6600
ref #6600

@pulpbot
Copy link
Member

pulpbot commented May 8, 2020

Attached issue: https://pulp.plan.io/issues/6600

Comment on lines 60 to 95
with open(f"{plugin_path}/setup.py", "rt") as setup_file:
setup_lines = setup_file.readlines()

with open(f"{plugin_path}/setup.py", "wt") as setup_file:
for line in setup_lines:
if "pulpcore" in line and "entry_points" not in line and "pulpcore" not in release_path:
sep = "'" if len(line.split('"')) == 1 else '"'
for word in line.split(sep):
if "pulpcore" in word:
pulpcore_word = word

line = line.replace(
pulpcore_word, f"pulpcore>={pulpcore_version},<{next_pulpcore_version}"
)

setup_file.write(line)
Copy link
Member Author

Choose a reason for hiding this comment

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

Still need to do it, as bump2version does not update pulpcore requirement

Comment on lines 89 to 123
with open(f"{plugin_path}/setup.py", "wt") as setup_file:
for line in setup_lines:
if "pulpcore" in line and "entry_points" not in line and "pulpcore" not in release_path:
sep = "'" if len(line.split('"')) == 1 else '"'
for word in line.split(sep):
if "pulpcore" in word:
pulpcore_word = word

line = line.replace(pulpcore_word, f"pulpcore>={pulpcore_version}")

setup_file.write(line)
Copy link
Member Author

Choose a reason for hiding this comment

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

Still need to do it, as bump2version does not update pulpcore requirement

@fao89 fao89 force-pushed the 6600 branch 3 times, most recently from 005b89c to 3673cb7 Compare May 8, 2020 20:37
if ".travis" in release_path:
plugin_path = os.path.dirname(release_path)

release_part = sys.argv[1]
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about converting this to use argparse directly? The benefit is that it provides auto-documenting --help usage on the command which could replace the manually maintained one at the top of this file for example. Also it provides validation.

@fao89 fao89 force-pushed the 6600 branch 2 times, most recently from 1144f0b to 9ccf117 Compare May 11, 2020 17:57
setup.py Outdated
@@ -7,9 +7,13 @@
with open("README.rst") as f:
long_description = f.read()

version = {}
with open("pulp_file/__init__.py") as fp:
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't bump2verison handle this de-duplication for us? I think this is option (3) listed here but we're also using option (2) from here https://packaging.python.org/guides/single-sourcing-package-version/ Can bump2version be configured to set it directly in both places?

Copy link
Member Author

@fao89 fao89 May 11, 2020

Choose a reason for hiding this comment

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

I used this approach because when I tried to use bump2version pointing to setup.py it changed the plugin version and also the requirements version.

Example:

requirements = [requests>=3.0.1]
setup(version=3.0.1

it would replace 3.0.1 on these 2 places

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see what you mean. This makes sense then.

Copy link
Member

Choose a reason for hiding this comment

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

Not being able to read the version of a package in setup.py itself feels awkward. So not as part of this PR, but maybe in future work the requirements can be moved to its own file in the repository that setup.py loads (similar to the other requirements files we have and then bump2version can handle it all.

help=textwrap.dedent(
"""\
Whether the release should be major, minor or patch.

Copy link
Member

Choose a reason for hiding this comment

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

What is the motivation for making these triple quote strings with dedent?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Is using simple strings an option like the examples in argparse? https://docs.python.org/3/library/argparse.html

Copy link
Member Author

Choose a reason for hiding this comment

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

it works with simple strings:

❯ python .travis/release.py --help 
usage: release.py [-h] release_part lower_pulpcore_version upper_pulpcore_version

Start the release process.

positional arguments:
  release_part          Whether the release should be major, minor or patch.
  lower_pulpcore_version
                        Lower bound of pulpcore requirement.
  upper_pulpcore_version
                        Upper bound of pulpcore requirement.

optional arguments:
  -h, --help            show this help message and exit

@fao89 fao89 marked this pull request as ready for review May 11, 2020 19:58

$ python .travis/release.py

Args:
Copy link
Member

Choose a reason for hiding this comment

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

With argparse being self-documenting I don't think having a second copy here is more helpful than the risk it will become out of date.

Copy link
Member Author

Choose a reason for hiding this comment

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

agreed, I put it here because I didn't have it documented before as argparse did

upper_pulpcore_version - upper bound of pulpcore requirement

Example:
setup.py on plugin before script:
Copy link
Member

Choose a reason for hiding this comment

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

These examples are maybe good if they were moved into argparse as part of the heltext response also.

setup.py Outdated
@@ -17,7 +18,7 @@
author_email="pulp-dev@redhat.com",
url="https://pulpproject.org/",
python_requires=">=3.6",
install_requires=requirements,
install_requires=requirements, # Currently it cannot deal with hashes
Copy link
Member Author

Choose a reason for hiding this comment

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

From what I researched, currently, setuptools does not support hashes on install_requires yet

Copy link
Member

Choose a reason for hiding this comment

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

Since we're not using the hashes I think this comment doesn't make sense anymore.

Comment on lines 1 to 10
#
# This file is autogenerated by pip-compile
# To update, run:
#
# pip-compile --generate-hashes --output-file=hashed_requirements.txt
#
aiodns==2.0.0 \
--hash=sha256:815fdef4607474295d68da46978a54481dd1e7be153c7d60f9e72773cd38d77d \
--hash=sha256:aaa5ac584f40fe778013df0aa6544bf157799bd3f608364b451840ed2c8688de \
# via pulpcore
Copy link
Member Author

Choose a reason for hiding this comment

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

I kept this file here because I thought it would be cool to document on the installation steps we have requirements with hashes

Copy link
Member

Choose a reason for hiding this comment

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

This is great, but how can we maintain it? If it becomes out of date that would be a problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

true, I think we have to remove it for now, as it is not supported yet on install_requires from setuptools

Copy link
Member

Choose a reason for hiding this comment

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

Even though it's sweet, I agree.

@fao89
Copy link
Member Author

fao89 commented May 12, 2020

I created this PoC of moving to poetry:
#391

@fao89 fao89 requested a review from bmbouter May 12, 2020 19:12
Copy link
Member

@bmbouter bmbouter left a comment

Choose a reason for hiding this comment

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

Thank you @fao89 !

@fao89 fao89 force-pushed the 6600 branch 2 times, most recently from 0d09f7c to 84ccfb6 Compare May 27, 2020 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants