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

enable darwin for the node installation in the CI #51705

Merged
merged 7 commits into from
Feb 3, 2020

Conversation

v1v
Copy link
Member

@v1v v1v commented Nov 26, 2019

Summary

Enable darwin OS support when using the scripts for the CI. Therefore, anyone could reproduce the same steps locally independently of the OS.

$ system_profiler SPSoftwareDataType
Software:

    System Software Overview:

      System Version: macOS 10.14.3 (18D42)
      Kernel Version: Darwin 18.2.0
      Boot Volume: Macintosh HD
      Boot Mode: Normal
      Computer Name: v1v-box
      User Name: Victor Edgar Martinez Rubio (vmartinez)
      Secure Virtual Memory: Enabled
      System Integrity Protection: Enabled
      Time since boot: 3:20

$ sw_vers
ProductName:	Mac OS X
ProductVersion:	10.14.3
BuildVersion:	18D42

$ uname -a
Darwin Victors-MacBook-Pro.local 18.2.0 Darwin Kernel Version 18.2.0: Thu Dec 20 20:46:53 PST 2018; root:xnu-4903.241.1~1/RELEASE_X86_64 x86_64

$ uname
Darwin

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@v1v v1v self-assigned this Nov 26, 2019
@v1v v1v added the automation label Nov 26, 2019
@v1v v1v marked this pull request as ready for review November 26, 2019 12:00
@v1v v1v requested a review from a team as a code owner November 26, 2019 12:00
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@mistic mistic left a comment

Choose a reason for hiding this comment

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

I like the idea, but It would be better to have the opinion of @brianseeders and @spalger here

@spalger spalger added release_note:skip Skip the PR/issue when compiling release notes Team:Operations Team label for Operations Team v6.8.5 v7.3.2 v7.4.3 v7.5.1 v7.6.0 v8.0.0 labels Dec 5, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-operations (Team:Operations)

@spalger
Copy link
Contributor

spalger commented Dec 5, 2019

@elasticmachine merge upstream

@spalger
Copy link
Contributor

spalger commented Dec 5, 2019

I just want to say, I don't recommend running the CI script on your local machine, they're designed to run on single-purpose machines and don't take care to protect user data or be a "good citizen". Please be careful, continue at your own risk :)

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

Well, it's always a good option to have the CI been reproducible in local, otherwise developers won't be able to check if a failure on CI is real or it's something on their own.

If the build does something weird with data, then it must bee encapsulated at CI level, not on build scripts.

@@ -71,7 +73,7 @@ if [[ "$OS" == "win" ]]; then
nodeUrl="https://us-central1-elastic-kibana-184716.cloudfunctions.net/kibana-ci-proxy-cache/dist/v$nodeVersion/node-v$nodeVersion-win-x64.zip"
else
nodeBin="$nodeDir/bin"
nodeUrl="https://us-central1-elastic-kibana-184716.cloudfunctions.net/kibana-ci-proxy-cache/dist/v$nodeVersion/node-v$nodeVersion-linux-x64.tar.gz"
nodeUrl="https://us-central1-elastic-kibana-184716.cloudfunctions.net/kibana-ci-proxy-cache/dist/v$nodeVersion/node-v$nodeVersion-${OS}-x64.tar.gz"
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the URL will also work with Windows, mind moving the nodeUrl outside the condition block so we only have a single place where nodeUrl is set?

Copy link
Member Author

Choose a reason for hiding this comment

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

See 003a361

@tylersmalley
Copy link
Contributor

I also want to echo what Spencer mentioned - these setup scripts are for CI and should not be run on your machine. If you want to reproduce what CI is running, you can run the underlying commands on the repository which will build/test.

@v1v
Copy link
Member Author

v1v commented Dec 9, 2019

I agree to disagree, :)
Based on my experience, the more simple and agnostic the scripts can be the easier to debug independently of the CI system underneath. This particular PR was based on my very first contact with the Kibana CI.
I do understand that the privileges and so on might be something to double-check after using the CI scripts, but as a first-time contributor I'd say I don't know of what other implications might be involved, so bear with me :)

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@spalger
Copy link
Contributor

spalger commented Dec 10, 2019

You're welcome to run whatever script you'd like on your computer, I just don't recommend it because we don't write them with the intention of being runnable on your own machine. So, run at your own risk I guess.

@v1v
Copy link
Member Author

v1v commented Jan 7, 2020

@elasticmachine merge upstream

@v1v
Copy link
Member Author

v1v commented Feb 3, 2020

Are there any concerns regarding this PR? Otherwise, when could it be merged? Thanks

@jbudz
Copy link
Member

jbudz commented Feb 3, 2020

good to go - i'll merge once we go green

@jbudz
Copy link
Member

jbudz commented Feb 3, 2020

@v1v do you need this back further than 7.7? dropping the 7.6 labels would help the inevitable feature freeze discussion. alternatively we could wait for 7.6.1

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@jbudz jbudz merged commit c76519e into elastic:master Feb 3, 2020
jbudz pushed a commit that referenced this pull request Feb 3, 2020
* enable darwin for the node installation in the CI

* refactor: avoid hardcode strings and customise based on the OS flavour

* fix classifier

Co-authored-by: Elastic Machine <[email protected]>
@jbudz
Copy link
Member

jbudz commented Feb 3, 2020

I pushed this to 7.7 for now. let me know if we need it further

@v1v
Copy link
Member Author

v1v commented Feb 4, 2020

No, I don't think we need any backport for previous versions. Thanks so much

@v1v v1v deleted the feature/support-darwin-in-ci branch February 4, 2020 11:27
jfsiii pushed a commit to jfsiii/kibana that referenced this pull request Feb 4, 2020
* enable darwin for the node installation in the CI

* refactor: avoid hardcode strings and customise based on the OS flavour

* fix classifier

Co-authored-by: Elastic Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automation release_note:skip Skip the PR/issue when compiling release notes Team:Operations Team label for Operations Team v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants