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.

Sign up for GitHub

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

Jump to bottom

[1.24] fix: Add python-requests to the list of required RPMs #3371

Merged
merged 1 commit into from
Jan 19, 2024
Merged

[1.24] fix: Add python-requests to the list of required RPMs #3371

merged 1 commit into from
Jan 19, 2024

Conversation

jirihnidek
Copy link
Contributor

  • Card ID: RHEL-11947
  • RPM package python-requests was not required, but "requests" Python package is used by auto-registration code

@ptoscano ptoscano changed the title fix: 1.28 Add python-requests to the list of required RPMs 1.24] fix: Add python-requests to the list of required RPMs Jan 17, 2024
@ptoscano ptoscano changed the title 1.24] fix: Add python-requests to the list of required RPMs [1.24] fix: Add python-requests to the list of required RPMs Jan 17, 2024
@jirihnidek jirihnidek force-pushed the jhnidek/1.24-RHEL-11947_add_python_requests branch 4 times, most recently from 9371051 to fbf65fa Compare January 17, 2024 14:37
@ptoscano
Copy link
Contributor

PR #3372 fixed the two issues present (the real "problem" what the failed pip installation); thanks both @m-horky and Jirka for the two changes needed!

Jirka: can you please rebase on top of the 1.24 branch, leaving only the dependency fix? Also, please the "1.28" typo in the commit message, since you are there. Thanks!

@jirihnidek
fix: 1.24 Add python-requests to the list of required RPMs
6442c78 
* Card ID: RHEL-11947
* RPM package python-requests was not required, but "requests"
  Python package is used by auto-registration code
@jirihnidek jirihnidek force-pushed the jhnidek/1.24-RHEL-11947_add_python_requests branch from 8da9e3f to 0e942dd Compare January 18, 2024 10:53
@jirihnidek
Copy link
Contributor Author

@ptoscano FYI: PR updated

ptoscano
ptoscano requested changes Jan 18, 2024
View reviewed changes
Copy link
Contributor

@ptoscano ptoscano left a comment

Choose a reason for hiding this comment

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

Thanks for the rebase.

Please note that the CI changes are not needed: there is no need to get the sources as git repository, and if you notice #3372 then you can see things were fixed by that PR already. Hence, please drop the "fix: CI: setup git repository properly" commit.

@jirihnidek
Copy link
Contributor Author

As you can see in the following job there were issues related to the fact that directory created from downloaded compressed archive wasn't real git repository.

E.g. this job: https://github.com/candlepin/subscription-manager/actions/runs/7554710695/job/20568141363?pr=3371

fatal: Not a git repository (or any parent up to mount point /__w)
Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).

I would keep this commit as part of this PR for this reason.

@ptoscano
Copy link
Contributor

As you can see in the following job there were issues related to the fact that directory created from downloaded compressed archive wasn't real git repository.

As I said, the nosetest job does not really need the sources as git repository.

E.g. this job: https://github.com/candlepin/subscription-manager/actions/runs/7554710695/job/20568141363?pr=3371

fatal: Not a git repository (or any parent up to mount point /__w)
Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).

IMHO that was another consequence of the failed pip installation.

I would keep this commit as part of this PR for this reason.

I'm pretty sure there is no need for it -- again, see #3372.

@jirihnidek
Copy link
Contributor Author

IMHO that was another consequence of the failed pip installation.

No, it wasn't.

@ptoscano
Copy link
Contributor

There should still be no need to have the sources as git repository, and both the build and the execution of the test suite do not need a git repository.

@jirihnidek jirihnidek force-pushed the jhnidek/1.24-RHEL-11947_add_python_requests branch from 0e942dd to 6442c78 Compare January 19, 2024 11:19
ptoscano
ptoscano approved these changes Jan 19, 2024
View reviewed changes
Copy link
Contributor

@ptoscano ptoscano left a comment

Choose a reason for hiding this comment

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

thanks!

@ptoscano ptoscano merged commit 38eeeaf into subscription-manager-1.24 Jan 19, 2024
2 checks passed
@ptoscano ptoscano deleted the jhnidek/1.24-RHEL-11947_add_python_requests branch January 19, 2024 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ptoscano ptoscano ptoscano approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jirihnidek @ptoscano