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

packaging.version related changes: chatch the proper exception and switch the priority #3383

Merged
merged 3 commits into from
Oct 16, 2023

Conversation

hroncok
Copy link
Contributor

@hroncok hroncok commented Oct 13, 2023

I am sending this as a draft as I won't be able to finish the checklist right away. Please do let me know if this seems OK to you and if so, I can amend the magical phrase to commit messages and rewrap them at 72 characters.


Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines

  • Is the commit message split over multiple lines and hard-wrapped at 72 characters?
  • Is the subject and message clear and concise?
  • Does the subject start with [plugin_name] if submitting a plugin patch or a [section_name] if part of the core sosreport code?
  • Does the commit contain a Signed-off-by: First Lastname [email protected]?
  • Are any related Issues or existing PRs properly referenced via a Closes (Issue) or Resolved (PR) line?

@hroncok hroncok changed the title Packaging packaging.version related changes: chatch the proper exception and switch the priority Oct 13, 2023
@packit-as-a-service
Copy link

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

  • sudo yum install -y dnf-plugins-core on RHEL 8
  • sudo dnf install -y dnf-plugins-core on Fedora
  • dnf copr enable packit/sosreport-sos-3383
  • And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

@arif-ali
Copy link
Member

arif-ali commented Oct 13, 2023

This was discussed in #3126, and the preference of using pkg_resources, as packaging causes cross repo dependency in RHEL

@hroncok
Copy link
Contributor Author

hroncok commented Oct 13, 2023

I actually want to get rid of setuptools from BaseOS in RHEL 10+.

@arif-ali
Copy link
Member

arif-ali commented Oct 13, 2023

I actually want to get rid of setuptools from BaseOS in RHEL 10+.

OK, we at Canonical will need to be aware of this change, so that the relevant changes can be made too for Ubuntu OSs too. We have pythnon3-packaging already available in main repo, which is typically the requirement for dependencies, going back to 20.04. So, from our perspective, I don't think it will cause an issue

@pmoravec
Copy link
Contributor

Generally ACK, just unsure about the flake failure (https://github.com/sosreport/sos/pull/3383/checks?check_run_id=17680606514):

sos/utilities.py:28:5: F401 'pkg_resources.parse_version' imported but unused

Technically the error is right, practically utilities imports a Python class in some way, and that is used later at other places (https://github.com/search?q=repo%3Asosreport%2Fsos%20parse_version&type=code).

Utilities should be loaded prior any call of parse_version either way, but.. still I think it would be safer to keep the original utilities' method parse_version (calling some imported method of same name)..?

@hroncok
Copy link
Contributor Author

hroncok commented Oct 16, 2023

I've added a commit to please the linter in the way I consider "proper". If you do not like that, I can remove the two commits and leave the function definition as it was, because that's now what I came here to do anyway.

Getting SyntaxError from pkg_resources is unlikely.

I hereby declare that you can do whatever you want with this commit:

Signed-off-by: Miro Hrončok <[email protected]>
This seems a bit more straightforward and achieves the same result.

This also defines the API of sos.utilities to avoid
flake8 "imported but unused"

I hereby declare that you can do whatever you want with this commit:

Signed-off-by: Miro Hrončok <[email protected]>
pkg_resources bundle and call packaging.version internally anyway.

In RHEL 10 and Ubuntu, we have packaging in BaseOS/main.

I hereby declare that you can do whatever you want with this commit:

Signed-off-by: Miro Hrončok <[email protected]>
@hroncok
Copy link
Contributor Author

hroncok commented Oct 16, 2023

Rebased, hopefully, the commits now adhere to the rules.

@hroncok hroncok marked this pull request as ready for review October 16, 2023 08:40
Copy link
Member

@TurboTurtle TurboTurtle left a comment

Choose a reason for hiding this comment

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

Ack, though I should note that we were indeed seeing SyntaxError previously.

For future contributions, please note that we prefer [tags] in the commit title, as it makes groking through the git history much easier, but this change I'm not going to be a stickler about it.

@TurboTurtle TurboTurtle merged commit 5690401 into sosreport:main Oct 16, 2023
34 checks passed
@hroncok hroncok deleted the packaging branch October 27, 2023 12:36
@hroncok
Copy link
Contributor Author

hroncok commented Oct 27, 2023

A followup: #3398

arif-ali added a commit to arif-ali/sos that referenced this pull request Nov 14, 2023
Related: sosreport#3398
Related: sosreport#3383
Resolves: SET-326
Signed-off-by: Arif Ali <[email protected]>
TurboTurtle pushed a commit that referenced this pull request Nov 17, 2023
Related: #3398
Related: #3383
Resolves: SET-326
Signed-off-by: Arif Ali <[email protected]>
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.

4 participants