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

macos ansible setup improvements #3410

Closed
wants to merge 3 commits into from
Closed

Conversation

mahdipub
Copy link

@mahdipub mahdipub commented Feb 21, 2024

unixPB: This PR is amed to add Xcode_offline which installs xcode without need to use any other 3rd party resource. Also some minor code changes and improvement added (e.g. install brew)

Checklist
  • commit message has one of the standard prefixes
  • faq.md updated if appropriate
  • other documentation is changed or added (if applicable)
  • VPC/QPC not applicable for this PR

This PR is amed to add Xcode_offline which installs xcode without need to use any other 3rd party resource. Also some minor code changes and improvement added (e.g. install brew)
Copy link
Contributor

@karianna karianna left a comment

Choose a reason for hiding this comment

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

Some unhappy YAML lint

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.

I've added a couple of other people as reviewers who are more knowledgeable about the macos stuff than I am but from a general perspective:

  • At present we use the main.yml file for macos, so switching to use a separate one superficially feels like a slightly backwards step. Is it possible to integrate some of these changes into the normal execution of main.yml or is there a reason why you have chosen to do this as a separate top level file?
  • This seems to have quite significant changes to some of the roles such as the ant-contrib one - are they specific to the goal of this PR to improve usability of the macos playbooks?
  • Similarly this seems to change how things like the credentials are retrieved for the Jenkins_User role, which would be a change in behaviour.

Perhaps some of those things can be decoupled from this PR and proposed separately unless there is a reason they've all been put together.

@mahdipub
Copy link
Author

I've added a couple of other people as reviewers who are more knowledgeable about the macos stuff than I am but from a general perspective:

  • At present we use the main.yml file for macos, so switching to use a separate one superficially feels like a slightly backwards step. Is it possible to integrate some of these changes into the normal execution of main.yml or is there a reason why you have chosen to do this as a separate top level file?
  • This seems to have quite significant changes to some of the roles such as the ant-contrib one - are they specific to the goal of this PR to improve usability of the macos playbooks?
  • Similarly this seems to change how things like the credentials are retrieved for the Jenkins_User role, which would be a change in behaviour.

Perhaps some of those things can be decoupled from this PR and proposed separately unless there is a reason they've all been put together.

  • Yes. This file (including comments on the bottom) is only to show what are changes, and how does this whole works for macos. For sure we can combine them.
  • Yes. But at the same time they are general improvements in code and logic too. e.g in ant-contrib most of it is to introduce a new variable {{ ant_binary_file_name }} which lets avoid repetition of extracting the path every time we need.
  • Agreed.
  • Agreed. I can focus on xcode offline and then ask for the rest in separate PR.

@karianna
Copy link
Contributor

@mahdipub Can you resolve the linter fixes?

@mahdipub
Copy link
Author

Based on comments on slack, I will close this PR and split changes in 2~3 PRs. The one related to Xcode-offline and the other for brew is #3417.

@mahdipub mahdipub closed this Feb 27, 2024
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.

None yet

3 participants