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

aixPb: Put /usr/bin in front of PATH #3716

Merged
merged 2 commits into from
Aug 20, 2024

Conversation

Haroon-Khel
Copy link
Contributor

@Haroon-Khel Haroon-Khel commented Aug 15, 2024

  • commit message has one of the standard prefixes
  • faq.md updated if appropriate
  • other documentation is changed or added (if applicable)
  • playbook changes run through VPC or QPC (if you have access)
  • VPC/QPC not applicable for this PR
  • for inventory.yml changes, bastillion/nagios/jenkins updated accordingly

To escape the rebase hell that is #2288, ive simply created a new pr :)

Copy link
Member

@sxa sxa left a comment

Choose a reason for hiding this comment

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

Why is /etc in there? I can't think of anything we need in the PATH that would be in there

@Haroon-Khel
Copy link
Contributor Author

Not sure. I'll remove it

Copy link
Contributor

@steelhead31 steelhead31 left a comment

Choose a reason for hiding this comment

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

Looks ok.

Copy link
Member

@sxa sxa left a comment

Choose a reason for hiding this comment

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

We should also bear in mind that while this will be ok for new machines it likely won't change the existing machines which have PATH=/opt/freeware/bin at the start as most machines currently won't match this regex, so if we don't put another case in here for that we'll need to look at patching the existing machines up (as described in #2288 (comment)). Also noting that this regex to update the config will trigger on every playbook execution since it sets the path to something that has PATH=/usr/bin at the start. Something like @aixtools' proposal in #2288 (review) may be preferable, but I'm also ok with a one-off manual fixup.

@karianna
Copy link
Contributor

We should also bear in mind that while this will be ok for new machines it likely won't change the existing machines which have PATH=/opt/freeware/bin at the start as most machines currently won't match this regex, so if we don't put another case in here for that we'll need to look at patching the existing machines up (as described in #2288 (comment)). Also noting that this regex to update the config will trigger on every playbook execution since it sets the path to something that has PATH=/usr/bin at the start. Something like @aixtools' proposal in #2288 (review) may be preferable, but I'm also ok with a one-off manual fixup.

holding off merging until we raise any appropriate work items here for consistency.

@Haroon-Khel Haroon-Khel merged commit a5918a0 into adoptium:master Aug 20, 2024
7 checks passed
@Haroon-Khel
Copy link
Contributor Author

I've manually updated the PATH on the current machines to start with /usr/bin:/opt/IBM/xlC/13.1.3/bin:/opt/freeware/bin:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants