Hi @bmelbourne. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

/assign @Atoms

@bmelbourne Could you please cleanup your git history, kind of a mess right now 😄

/approve

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bmelbourne, Miouge1

The full list of commands accepted by this bot can be found here.

The pull request process is described here

@bmelbourne Could you please cleanup your git history, kind of a mess right now 😄

@floryut
Can you provide a recommendation as I've been asked to rebase with the master before the PR will be reviewed and now this appears to be the incorrect approach. I'm getting confused and frustrated by whats been happening recently.

I've generally worked on submitting PRs (within DevOps/Dev teams) which get reviewed and approved, and then merge/conflicts are resolved pror to the PR closure.

I didn't rebase on my first few PRs and that was the incorrect approach then, hence the confusion now.

Would this be the correct approach?

$ git checkout master
$ git fetch upstream
$ git rebase upstream/master
$ git push origin master
$ git checkout set-docker-cgroup-driver-systemd
$ git pull origin master

@bmelbourne Could you please cleanup your git history, kind of a mess right now 😄

@floryut
Can you provide a recommendation as I've been asked to rebase with the master before the PR will be reviewed and now this appears to be the incorrect approach. I'm getting confused and frustrated by whats been happening recently.

I've generally worked on submitting PRs (within DevOps/Dev teams) which get reviewed and approved, and then merge/conflicts are resolved pror to the PR closure.

I didn't rebase on my first few PRs and that was the incorrect approach then, hence the confusion now.

Would this be the correct approach?

$ git checkout master
$ git fetch upstream
$ git rebase upstream/master
$ git push origin master
$ git checkout set-docker-cgroup-driver-systemd
$ git pull origin master

Well that's kind of correct, but indeed before opening a PR I tried to switch to master, fetch spray, then reset --hard my master branch (well not needed but never sure if I push some commits or not) and then checkout -b from it 😄

If you are on your branch already and you have a conflict, fetch spray then rebase spray/master is enough.

As you did a pull with merge instead of a rebase, commits ID changed to preserve your history and so we see changes that won't be applied and commit that will be ignored, not an issue but not really great.

@bmelbourne Could you please cleanup your git history, kind of a mess right now 😄

@floryut
Can you provide a recommendation as I've been asked to rebase with the master before the PR will be reviewed and now this appears to be the incorrect approach. I'm getting confused and frustrated by whats been happening recently.
I've generally worked on submitting PRs (within DevOps/Dev teams) which get reviewed and approved, and then merge/conflicts are resolved pror to the PR closure.
I didn't rebase on my first few PRs and that was the incorrect approach then, hence the confusion now.
Would this be the correct approach?

$ git checkout master
$ git fetch upstream
$ git rebase upstream/master
$ git push origin master
$ git checkout set-docker-cgroup-driver-systemd
$ git pull origin master

Well that's kind of correct, but indeed before opening a PR I tried to switch to master, fetch spray, then reset --hard my master branch (well not needed but never sure if I push some commits or not) and then checkout -b from it 😄

If you are on your branch already and you have a conflict, fetch spray then rebase spray/master is enough.

As you did a pull with merge instead of a rebase, commits ID changed to preserve your history and so we see changes that won't be applied and commit that will be ignored, not an issue but not really great.

@floryut
Just to be clear, this was incorrect...

$ git pull --rebase upstream master

What I should be doing if I need to bring my local branch up to date with the upstream master is...

$ git fetch upstream master
$ git rebase upstream/master
<resolve any merge conflicts if necessary>
$ git push

@bmelbourne Could you please cleanup your git history, kind of a mess right now 😄

@floryut
Can you provide a recommendation as I've been asked to rebase with the master before the PR will be reviewed and now this appears to be the incorrect approach. I'm getting confused and frustrated by whats been happening recently.
I've generally worked on submitting PRs (within DevOps/Dev teams) which get reviewed and approved, and then merge/conflicts are resolved pror to the PR closure.
I didn't rebase on my first few PRs and that was the incorrect approach then, hence the confusion now.
Would this be the correct approach?

$ git checkout master
$ git fetch upstream
$ git rebase upstream/master
$ git push origin master
$ git checkout set-docker-cgroup-driver-systemd
$ git pull origin master

Well that's kind of correct, but indeed before opening a PR I tried to switch to master, fetch spray, then reset --hard my master branch (well not needed but never sure if I push some commits or not) and then checkout -b from it 😄
If you are on your branch already and you have a conflict, fetch spray then rebase spray/master is enough.
As you did a pull with merge instead of a rebase, commits ID changed to preserve your history and so we see changes that won't be applied and commit that will be ignored, not an issue but not really great.

@floryut
Just to be clear, this was incorrect...

$ git pull --rebase upstream master

What I should be doing if I need to bring my local branch up to date with the upstream master is...

$ git fetch upstream master
$ git rebase upstream/master
<resolve any merge conflicts if necessary>
$ git push

@floryut
I forgot to ask...what approach would you recommend to clean-up the git history?

In my local branch associated with the PR...

$ git rebase -i

and then squash all commits except for the earliest commit e.g. a1855ec which is the first one I originally created with the PR?

In this case ? Yes what I would do, hard reset the branch with master from spray, cherry pick then squash commits 👍

In this case ? Yes what I would do, hard reset the branch with master from spray, cherry pick then squash commits 👍

Ok I've done the following...

$ git fetch origin master
$ git reset --hard origin/HEAD
<overwrite with my changes only>
$ git commit -am "..."
$ git push --force

And the git history of my 4 outstanding PRs are looking much better now.

Can you take a quick look?

In this case ? Yes what I would do, hard reset the branch with master from spray, cherry pick then squash commits 👍

Ok I've done the following...

$ git fetch origin master
$ git reset --hard origin/HEAD
<overwrite with my changes only>
$ git commit -am "..."
$ git push --force

And the git history of my 4 outstanding PRs are looking much better now.

Can you take a quick look?

That's way better :) thanks man

Good job with this one, just to be sure

      {%- elif ansible_os_family in ["Coreos", "Container Linux by CoreOS", "Flatcar", "Flatcar Container Linux by Kinvolk"] -%}
      {%- elif ansible_os_family in ["Flatcar Container Linux by Kinvolk"] -%}

ansible_is_family doesn't need "Flatcar" ? only "Flatcar Container Linux by Kinvolk" is returned ? (for all versions/use case etc..)

Good job with this one, just to be sure

      {%- elif ansible_os_family in ["Coreos", "Container Linux by CoreOS", "Flatcar", "Flatcar Container Linux by Kinvolk"] -%}
      {%- elif ansible_os_family in ["Flatcar Container Linux by Kinvolk"] -%}

ansible_is_family doesn't need "Flatcar" ? only "Flatcar Container Linux by Kinvolk" is returned ? (for all versions/use case etc..)

@floryut
Correct, I tested the latest stable, beta, alpha and edge versions of Flatcar virtualbox boxes.

There might be older versions where ansible_os_family returns a different string but it'll be impossible to cover all these combinations. If any users encounter this scenario then they'll have to use an earlier version of kubespray until they can upgrade to the latest version of Flatcar. This would be the same situation for any component which is upgraded or end-of-life.

Agreed with you no real point to cover every older versions
/lgtm