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

feat(os): improve macos & linux support #26

Merged
merged 5 commits into from
Jan 25, 2020

Conversation

noelmcloughlin
Copy link
Member

@noelmcloughlin noelmcloughlin commented Aug 17, 2019

This PR has some improvements and fixes

  1. Set and use rootuser and rootgroup per template formula (macos/bsd)
  2. Clarify GOPATH because its important - README special note
  3. Improve linux alternatives yaml handling and SLS updates
  4. Fix some issues with file/default/golang.sh.jinja

@myii
Copy link
Member

myii commented Aug 17, 2019

@noelmcloughlin After the recent batch of yamllint PRs (see saltstack-formulas/template-formula#159 as a starting point), the template-formula provides this functionality at the end of map.jinja instead. That's why the lint stage (will be) failing here.

@noelmcloughlin noelmcloughlin changed the title fix(config): use rootuser/rootgroup & update readme feat(os): improve macos & linux support Aug 18, 2019
@noelmcloughlin
Copy link
Member Author

Thanks @myii I fixed things up and verified on Ubuntu 18.04

          ID: golang-package-archive-install-file-directory
    Function: file.directory
        Name: /usr/local/go1.10.1.linux-amd64
      Result: True
     Comment: Directory /usr/local/go1.10.1.linux-amd64 updated
     Started: 17:50:43.812630
    Duration: 4258.498 ms
     Changes:   
              ----------
              mode:
                  0755
----------
          ID: golang-package-archive-install-file-directory
    Function: file.directory
        Name: /usr/local/golang/packages
      Result: True
     Comment: The directory /usr/local/golang/packages is in the correct state
     Started: 17:50:48.071336
    Duration: 0.89 ms
     Changes:   
----------
          ID: golang-package-archive-install-archive-extracted
    Function: archive.extracted
        Name: /usr/local/go1.10.1.linux-amd64
      Result: True
     Comment: All files in archive are already present
     Started: 17:50:48.072617
    Duration: 4367.464 ms
     Changes:   
----------
          ID: golang-package-archive-install-home-alternative-install
    Function: alternatives.install
        Name: golang-home
      Result: True
     Comment: Alternative /usr/local/go1.10.1.linux-amd64/go for golang-home is already registered
     Started: 17:50:52.440274
    Duration: 7.784 ms
     Changes:   
----------
          ID: golang-package-archive-install-godoc-alternative-install
    Function: alternatives.install
        Name: link-godoc
      Result: True
     Comment: Alternative /usr/local/go1.10.1.linux-amd64/go/bin/godoc for link-godoc is already registered
     Started: 17:50:52.448597
    Duration: 7.766 ms
     Changes:   
----------
          ID: golang-package-archive-install-gofmt-alternative-install
    Function: alternatives.install
        Name: link-gofmt
      Result: True
     Comment: Alternative /usr/local/go1.10.1.linux-amd64/go/bin/gofmt for link-gofmt is already registered
     Started: 17:50:52.456634
    Duration: 7.748 ms
     Changes:   
----------
          ID: golang-package-archive-install-go-alternative-install
    Function: alternatives.install
        Name: link-go
      Result: True
     Comment: Alternative /usr/local/go1.10.1.linux-amd64/go/bin/go for link-go is already registered
     Started: 17:50:52.464646
    Duration: 7.79 ms
     Changes:   
----------
          ID: golang-config-file-managed-environ_file
    Function: file.managed
        Name: /etc/default/golang.sh
      Result: True
     Comment: File /etc/default/golang.sh updated
     Started: 17:50:52.472695
    Duration: 32.949 ms
     Changes:   
              ----------
              diff:
                  --- 
                  +++ 
                  @@ -3,8 +3,13 @@
                   # Your changes may be overwritten.
                   ########################################################################
                   
                  -export GOROOT=/usr/local/go
                  -export GOPATH=/usr/local/golang/packages
                  -export GOBASE=/usr/local/go1.10.1.linux-amd64/go
                  -export PATH=$PATH:$GOROOT/bin:$GOBASE/bin
                  -export PATH=${PATH}:/usr/local/go1.10.1.linux-amd64
                  +##portable shell syntax
                  +
                  +[ -n "${GOROOT}" ] && export GOROOT=${GOROOT}
                  +[ -n "/usr/local/go1.10.1.linux-amd64" ] && export GOROOT=/usr/local/go1.10.1.linux-amd64
                  +
                  +[ -n "${GOPATH}" ] && export GOPATH=${GOPATH}
                  +[ -n "/usr/local/golang/packages" ] && export GOPATH=/usr/local/golang/packages
                  +
                  +export PATH=$PATH:${GOROOT}/go/bin
                  +export PATH=${PATH}:/usr/local/go1.10.1.linux-amd64/go/bin
----------
          ID: golang-package-archive-install-home-alternative-install
    Function: cmd.run
        Name: update-alternatives --install /usr/local/go golang-home /usr/local/go1.10.1.linux-amd64/go 1000
      Result: True
     Comment: onlyif condition is false
     Started: 17:50:52.505840
    Duration: 0.429 ms
     Changes:   
----------
          ID: golang-package-archive-install-home-alternative-set
    Function: alternatives.set
        Name: golang-home
      Result: True
     Comment: Alternative for golang-home already set to /usr/local/go1.10.1.linux-amd64/go
     Started: 17:50:52.506404
    Duration: 4.273 ms
     Changes:   
----------
          ID: golang-package-archive-install-go-alternative-install
    Function: cmd.run
        Name: update-alternatives --install /usr/bin/go link-go /usr/local/go1.10.1.linux-amd64/go/bin/go 1000
      Result: True
     Comment: onlyif condition is false
     Started: 17:50:52.510814
    Duration: 0.431 ms
     Changes:   
----------
          ID: golang-package-archive-install-go-alternative-set
    Function: alternatives.set
        Name: link-go
      Result: True
     Comment: Alternative for link-go already set to /usr/local/go1.10.1.linux-amd64/go/bin/go
     Started: 17:50:52.511593
    Duration: 4.075 ms
     Changes:   
----------
          ID: golang-package-archive-install-godoc-alternative-install
    Function: cmd.run
        Name: update-alternatives --install /usr/bin/godoc link-godoc /usr/local/go1.10.1.linux-amd64/go/bin/godoc 1000
      Result: True
     Comment: onlyif condition is false
     Started: 17:50:52.515809
    Duration: 0.372 ms
     Changes:   
----------
          ID: golang-package-archive-install-godoc-alternative-set
    Function: alternatives.set
        Name: link-godoc
      Result: True
     Comment: Alternative for link-godoc already set to /usr/local/go1.10.1.linux-amd64/go/bin/godoc
     Started: 17:50:52.516469
    Duration: 4.187 ms
     Changes:   
----------
          ID: golang-package-archive-install-gofmt-alternative-install
    Function: cmd.run
        Name: update-alternatives --install /usr/bin/gofmt link-gofmt /usr/local/go1.10.1.linux-amd64/go/bin/gofmt 1000
      Result: True
     Comment: onlyif condition is false
     Started: 17:50:52.520795
    Duration: 0.368 ms
     Changes:   
----------
          ID: golang-package-archive-install-gofmt-alternative-set
    Function: alternatives.set
        Name: link-gofmt
      Result: True
     Comment: Alternative for link-gofmt already set to /usr/local/go1.10.1.linux-amd64/go/bin/gofmt
     Started: 17:50:52.521450
    Duration: 4.147 ms
     Changes:   

Summary for local
-------------
Succeeded: 16 (changed=2)
Failed:     0
-------------
Total states run:     16



vagrant@ubuntu1804:~$ go env GOPATH
/home/vagrant/go
vagrant@ubuntu1804:~$ go env GOROOT
/usr/local/go
vagrant@ubuntu1804:~$ which go
/usr/bin/go

@noelmcloughlin
Copy link
Member Author

Fixed integration tests. BTW, this PR has no breaking changes.

@noelmcloughlin
Copy link
Member Author

noelmcloughlin commented Aug 18, 2019

Fixed comment syntax so lint passes.

Regardless of this PR I am concerned that GOPATH is exported incorrectly in this formula.
<moved text to #28 >

Hi @jackscott , could you help.

@noelmcloughlin
Copy link
Member Author

noelmcloughlin commented Aug 18, 2019

@noelmcloughlin
Copy link
Member Author

To unblock this PR I raised #28

@myii
Copy link
Member

myii commented Aug 21, 2019

Sorry for the delay, @noelmcloughlin. There looks like quite a few changes here, so my plan is to run this through a little process I have locally, to compare before and after. Once I have the results, I'll share them with you here.

@myii
Copy link
Member

myii commented Aug 21, 2019

@noelmcloughlin Just to add, this may take me a day or two but I'll definitely come back to this (bookmarked). Please feel free to ping me if I take too long.

@myii
Copy link
Member

myii commented Aug 24, 2019

@noelmcloughlin I've got some results that I'll be sharing with you soon. I haven't forgotten about you!

@myii
Copy link
Member

myii commented Sep 11, 2019

@noelmcloughlin It only took me another 18 days! Sorry about that. So here it is:

If you can look through that diff and confirm that's the exact map changes you were expecting, then we're a step closer to getting this merged. Just to be clear, I've run a yaml_dump state locally, that extracts the map during the Travis run, for each instance. I did that before and after and that gives us a diff of what has changed in the proposed PR. What do you think about having such a feature used in our formulas?

@noelmcloughlin
Copy link
Member Author

Hi @myii I have looked through the diff and the changes are as expected.

This yaml_dump_diff feature could be useful.
It's hard to judge in this case because osfamilymap.yaml is empty for Linux OSes.

@myii
Copy link
Member

myii commented Sep 11, 2019

Hi @myii I have looked through the diff and the changes are as expected.

@noelmcloughlin Great. I'd be more comfortable with a second set of eyes to look through the changes. Any suggestions on who to ping?

@noelmcloughlin
Copy link
Member Author

@jackscott I guess.

@noelmcloughlin
Copy link
Member Author

@myii can we merge this - no second pair of eyes is available ;-)

@myii
Copy link
Member

myii commented Dec 3, 2019

@noelmcloughlin Just a brief point. We don't need to include these files any more since they have been centralised into the https://github.com/saltstack-formulas/.github repo:

  • .github/ISSUE_TEMPLATE/bug_report.md
  • .github/ISSUE_TEMPLATE/feature_request.md

@noelmcloughlin noelmcloughlin removed the request for review from jackscott January 25, 2020 18:26
@noelmcloughlin
Copy link
Member Author

selfie merging as this is open a long time without further comments.
I squashed some commits and fixed rubocop problems.
Any issues can be addressed in followup PR.

@noelmcloughlin noelmcloughlin merged commit fecf92a into saltstack-formulas:master Jan 25, 2020
@noelmcloughlin noelmcloughlin deleted the gopath branch January 25, 2020 21:33
@saltstack-formulas-travis

🎉 This PR is included in version 0.6.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

3 participants