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

init.sh and manage-common.sh fixes for successful initialization #1457

Closed
wants to merge 2 commits into from

Conversation

LinaresToine
Copy link

PR to fix initialization issues pointed out by @todor-ivanov in dmwm/WMCore#11890 (comment)

@todor-ivanov
Copy link
Contributor

todor-ivanov commented Mar 18, 2024

@LinaresToine your PR is trying to revert changes from another one:
https://github.com/dmwm/CMSKubernetes/pull/1456/files

This may have happened because you forgot to update your local branch before creating the PR.
Could you please first fetch the master branch and fork from there before you create your PR so that you do not try to revert other changes.

@LinaresToine
Copy link
Author

@todor-ivanov I fixed the PR.

Copy link
Contributor

@todor-ivanov todor-ivanov left a comment

Choose a reason for hiding this comment

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

Hi @LinaresToine The changes look good. There was only one extra line which I commented inline. Please take a look.

docker/pypi/wmagent/init.sh Outdated Show resolved Hide resolved
Copy link
Author

Choose a reason for hiding this comment

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

The _check_mounts() function crashes without the return true statement in the beginning of the function.

It was one of the changes I was attempting to revert in the original PR. @todor-ivanov

Copy link
Contributor

@todor-ivanov todor-ivanov Mar 20, 2024

Choose a reason for hiding this comment

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

@LinaresToine I fail to relate this message to any of the action we took last time. Maybe it was something from an earlier stage. Could you please remind me?

Copy link
Author

Choose a reason for hiding this comment

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

Indeed, we never talked about this part. What I see when running the init.sh script in this PR is that the _check_mounts() function is returning false. However, the version of the script we were using during our last meeting has that function returning true by force, so we didnt see the issue then

Copy link
Contributor

Choose a reason for hiding this comment

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

hi @LinaresToine thanks for reminding me that ... I'll see what can be done.

@amaltaro
Copy link
Contributor

@todor-ivanov @LinaresToine is this PR still relevant? Perhaps it has all been integrated into #1466?

@LinaresToine
Copy link
Author

The changes in this PR have been implemented. I believe we can close this one.

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.

3 participants