-
Notifications
You must be signed in to change notification settings - Fork 101
removed build_utils dependency from ceph build pull request job #2398
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
base: main
Are you sure you want to change the base?
Conversation
e623748 to
a898d32
Compare
scripts/setup_deps.sh
Outdated
| elif command -v python2.7 > /dev/null; then | ||
| virtualenv -p python2.7 $path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know youre copying this from the other script now, but perhaps it's also a good time to drop python2 support entirely. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@phlogistonjohn good catch , yes we can do that. Actually I am also modifying something like I am removing the function keyword to make it POSIX compliant.
a898d32 to
9e5db2d
Compare
|
So I assume we are creating a smaller reusable chunk of code in setup_deps.sh here.
|
|
Hi @dmick , This PR is part of the ongoing refactor of the build_utils.sh script. A few key points: Script Name: I've named the new script setup_deps.sh since it focuses on setting up dependencies and can be reused across other jobs that require similar functionality. Open to suggestions if you have a better name in mind. Function Scope: All functions included in this script are migrated from build_utils.sh. There are no new additions—only modifications such as dropping Python 2 support and ensuring POSIX compliance. Future Use: The goal is to make this script reusable across multiple jobs. We're also working on logically splitting the functions—for example, separating setup-related logic from RHEL-specific build steps. |
|
script name: well, yes, as I said, it seems to be strictly Python dependencies, so I would definitely include that in the name, something like, oh, say, setup_python_deps.sh function movement: yes, but if there's no easy way to get diffs, there's no easy way to review it. Please provide text for review of the transported code. goals: good, but what I meant to try to encourage is looking at future users to see that you're doing things in a way compatible with their use, so that hopefully what changes in future is only the callers, and not the callee here |
9e5db2d to
db6060b
Compare
|
Hello @dmick
|
|
retest |
Signed-off-by: Adarsha Dinda <[email protected]>
db6060b to
bf32bcf
Compare
|
Out of fear of creating a split brain scenario, I still don't think duplicating code is the right answer for this endeavor. e.g.,
|
|
I'm assuming this is only affecting one job because we're attempting to stage one job, and then will shortly follow with the others, but yes, there's a potential for change in only one branch of the duplicated code. It's not ideal. |
Logs - https://jenkins.ceph.com/view/pull-requests/job/adarsha-ceph-build-pull-requests/1/