opened
ls42
username_0
Hey, found a tyop in the example after creating a cluster.
Fix sed command in example.
issue
created
oponder
username_1
@username_0 Thanks for your PR!

I think mac has a slightly weird version of `sed`.
If I run that (on macOS) I get: 

```
$ sed -i "s/YOUR_CLUSTER_BASE_DOMAIN/sdsn4.k8s.sample.io/" helloworld-manifest.yaml
sed: 1: "helloworld-manifest.yaml": extra characters at the end of h command
```

Does the above work for you? What platform are you using? 

Does anyone know an alternative that has more chance to take the same arguments?

Otherwise I can use platform detection in Happa and offer up different versions of the `sed` command. I'd rather avoid it though.
comment
created
ls42
username_0
@username_1 you're very welcome!

The command you mentioned works for me. If I'm not mistaken, macOS uses a BSD version of sed, and the GNU project somehow has a different version. I'm on Linux, details below. 

[I looked it up](http://man.openbsd.org/sed#i): BSD sed needs a filename after the `-i` command, for backups. It looks like there's no elegant way without platform detection.

```
clericbeast ~/gollum $ sed -i "s/YOUR_CLUSTER_BASE_DOMAIN/sdsn4.k8s.sample.io/" helloworld-manifest.yaml
clericbeast ~/gollum $ sed --version ; cat /etc/os-release 
sed (GNU sed) 4.7
Packaged by Debian
Copyright (C) 2018 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <https://gnu.org/licenses/gpl.html>.
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Written by Jay Fenlason, Tom Lord, Ken Pizzini,
Paolo Bonzini, Jim Meyering, and Assaf Gordon.
GNU sed home page: <https://www.gnu.org/software/sed/>.
General help using GNU software: <https://www.gnu.org/gethelp/>.
E-mail bug reports to: <bug-sed@gnu.org>.
NAME="Ubuntu"
VERSION="19.04 (Disco Dingo)"
ID=ubuntu
ID_LIKE=debian
PRETTY_NAME="Ubuntu 19.04"
VERSION_ID="19.04"
HOME_URL="https://www.ubuntu.com/"
SUPPORT_URL="https://help.ubuntu.com/"
BUG_REPORT_URL="https://bugs.launchpad.net/ubuntu/"
PRIVACY_POLICY_URL="https://www.ubuntu.com/legal/terms-and-policies/privacy-policy"
VERSION_CODENAME=disco
UBUNTU_CODENAME=disco
clericbeast ~/gollum $ grep sample.io helloworld-manifest.yaml 
  - host: helloworld.sdsn4.k8s.sample.io
clericbeast ~/gollum $ 
```
comment
created
ls42
username_0
It does work like this on GNU sed: `sed -i"" "s/YOUR_CLUSTER_BASE_DOMAIN/sdsn4.k8s.sample.io/" helloworld-manifest.yaml`. Maybe that works on macOS, too?
comment
created
oponder
username_1
Unfortunately not, just gave it try. I'll do the platform detection thing, we already have code for that in another part of the getting started guide, so it's not too much work to introduce that.
comment
created
oponder
username_1
@username_0: Unless you feel like taking your PR further? You can see the platform detection happening in 

https://github.com/giantswarm/happa/blob/master/src/components/getting-started/1_configure_kubectl.js

I would do it in a similar way to that page, so that we show the tabs (Mac, Linux, Windows), and then the appropriate sed command for each.

Totally no worries if that's a bit too much hassle and React / JSX slinging for what looked initially like a quick contribution.
comment
created
ls42
username_0
@username_1 I'm intrigued, let me have a look!
comment
created
ls42
username_0
See c4b4724e8f67c5ddf06ec67ac3b13b7e9d343bfe

Sorry, this is untested, I don't have the environment for React at
the moment. But maybe this already works as expected? I deliberately
left out the **Windows** tab since it would require changes in the first page
as well (adding `sed` to the *scoop install/update* part).
comment
created
oponder
username_1
@username_0  Let me preface this by saying I realise I'm asking for a lot of minutia from a friendly contributor. If at any point you think you want to move on from doing these little changes, just let me know. I'm happy to take over. 

With that out of the way. This looks good and works for me! 

I do have a few changes.

- There's some text changes I'm thinking of:

  Could you change the `p` tag from:

  ```
          <p>
            Next use <code>sed</code> to replace the placeholder{' '}
            <code>YOUR_CLUSTER_BASE_DOMAIN</code> with{' '}
            <code>{this.clusterBaseDomain()}</code>.
          </p>
  ```

  to (2 separate p tags):

  ```
          <p>
            Next replace the placeholder{' '}
            <code>YOUR_CLUSTER_BASE_DOMAIN</code> with{' '}
            <code>{this.clusterBaseDomain()}</code>.
          </p>

          <p>
            If you are on Linux or Mac OS you can use the command below to do this.
            Windows users willl have to use their favorite text editor and manually
            edit the <code>helloworld-manifest.yaml</code> file.
          </p>
  ```

- We also got a missing semicolon that our linter will complain about on line 36 of `2_simple_example.js`

- Also I would love it if you added yourself to the `CONTRIBUTORS.md`, unless you have any objection to that.

  If you're lost for words, it can be something like (inserted in alphabetical order),

  ```
  - [Stephan Brauer](https://github.com/username_0)
    - Discovered our sed command didn't work for linux users and contributed a thorough fix.
  ```

- And lastly can you acknowledge our DCO here: https://github.com/giantswarm/happa/blob/master/DCO
Replying with something like "I've read and agree with the DCO." is sufficient.

Thanks again!
comment
created
ls42
username_0
I've read and agree with the [DCO](https://github.com/giantswarm/happa/blob/master/DCO) :)
comment
created
ls42
username_0
@username_1 I'm seriously happy to help, and that you're trying to engage me fix this the proper way. Thank you!

I changed the parts you requested, although I took the liberty to change "Mac OS" to "macOS", I believe that's the official style. Let me know if I should change it back.

:v:
comment
created
ls42
username_0
Nevermind, changed it back to "Mac OS". I missed that you're writing it like that in other places, too.
comment
created
oponder
username_1
@username_0 looks good to me! I think there is a linter error still, sorry you can't see those, since our CI only runs for non forked pull requests.

Could you run `yarn run prettier` and commit the result, then we should have a fully good to go PR.

In the meanwhile I'll fiddle with our CI a bit, so that we can let forked PR's also get built. When that's done we should see the PR go fully green, and I'll merge it shortly after that.
comment
created
ls42
username_0
@username_1 done!
comment
created
oponder
username_1
Sorry this is taking a lil while, the CI changes are taking me a bit longer than I hoped.
comment
created
ls42
username_0
comment
created
oponder
username_1
Going to use my admin powers and merge this despite the CI error.

Building forked PR's requires a bit more work, but I don't want to let this linger because of that.

Thanks for your work @username_0
comment
created
ls42
username_0
That’s awesome, thanks!

I like how you encouraged me to go the extra mile, and it was very rewarding. 

Thanks again! I’ll be looking for the next opportunity to contribute :-)

Sent from my iPhone

>
comment