mongrelion / ansible-role-docker

Ansible role for installing Docker
MIT License
62 stars 39 forks source link

Ansible include module is deprecated in Ansible 2.4 and must be repla… #26

Closed Jaczel closed 6 years ago

Jaczel commented 6 years ago

Ansible include module is deprecated in Ansible 2.4 and must be replaced with either import_tasks for static inclusions or include_tasks for dynamic inclusions. As such roles/tasks/main.yml has been updated with include_tasks. Please test on previous version to Ansible 2.4 before merging.

mongrelion commented 6 years ago

@Jaczel it would be great if you yourself could test this on ansible version < 2.4. Otherwise it's going to take longer for the reviewer to actually test this and get your PR merged.

mongrelion commented 6 years ago

PRs won't be merged until we have a proper test suite to make sure that we don't break backwards compatibility.

paulfantom commented 6 years ago

Anisble include_tasks is supported only from version 2.4 and up so merging this PR will cause this role to be unusable on previous ansible versions. Currently due to deprecation of include module, ansible will warn about deprecation, but roles will work. Problem will be when ansible 2.8 will be released since this will be a version which removes include module.

mongrelion commented 6 years ago

Taking a look at Ansible's Release and maintenance policy, it goes:

Ansible supports the two most recent major stable releases

Currently the two major stable releases are 2.3 and 2.4 Dropping support for 2.3 would be premature, imo. I'd say let's wait until 2.5 is released but would also like to hear your thoughts, @paulfantom.

paulfantom commented 6 years ago

Ansible 2.2.3 is also supported right now (via https://docs.ansible.com/ansible/2.4/release_and_maintenance.html#release-status), but only security patches are applied.

If you want to implement this PR and be able to use this role with every supported ansible versions, you'll need to wait for about 6 months for ansible 2.6. After this release ansible 2.2 and 2.3 won't be supported.

paulfantom commented 6 years ago

Let's do it after #55 is merged.

mongrelion commented 6 years ago

Great to finally see this through. The other PR looks good, so let's get this merged once the other one has been figured out.

paulfantom commented 6 years ago

@Jaczel could you rebase onto master and fix conflicts?

Jaczel commented 6 years ago

Please see https://github.com/mongrelion/ansible-role-docker/pull/57 Edit: Apologies, I'm still new to using Git in a team and wasn't sure how to update the old branch to current, so I created a new branch, made the edits and created a new pull request. Please let me know if there is a better way to do this.

paulfantom commented 6 years ago

@Jaczel execute those in your repo:

git remote add upstream git@github.com:mongrelion/ansible-role-docker.git
git fetch upstream
git checkout ansible2.4
git rebase upstream/master

And follow instructions from git.

Also here is a good "cheat sheet": https://services.github.com/on-demand/downloads/github-git-cheat-sheet.pdf

This whole site is good to learn git: https://git-scm.com/doc

Jaczel commented 6 years ago

Thanks @paulfantom . That helped. Closing https://github.com/mongrelion/ansible-role-docker/pull/57

paulfantom commented 6 years ago

Whoooa, what just happened here? I'm wondering now if I wrote something wrong :thinking: This is completely rewriting git history.

Jaczel commented 6 years ago

Not too sure what I did wrong but here is a quick history of my commands: git remote add upstream git@github.com:mongrelion/ansible-role-docker.git git fetch upstream git checkout ansible2.4 git rebase upstream/master vim main.yml -- fixed conflicts removing old code from 81 commits ago and updating to include_tasks git add main.yml git rebase --continue git push -- received an error about commit history git pull -- this is where I imagine I made my mistake vim main.yml -- fixed conflicts again git commit -m "Merged and fixed conflicts" git push Hope that helps. Sorry for the inconvenience.

paulfantom commented 6 years ago

Ok, so due to github features, especially allowance to edit branches mentioned in PR, I was able to reset everything and rebase this. I did it only because I was curious if something I wrote was wrong. It wasn't.

@Jaczel I wrote that you need to follow what git command is saying. After running git rebase upstream/master it should tell you exactly this:

First, rewinding head to replay your work on top of it...
Applying: Ansible include module is deprecated in Ansible 2.4 and must be replaced with either `import_tasks` for static inclusions or `include_tasks` for dynamic inclusions. As such roles/tasks/main.yml has been updated with `include_tasks`. Please test on previous version to Ansible 2.4 before merging.
Using index info to reconstruct a base tree...
M       tasks/main.yml
Falling back to patching base and 3-way merge...
Auto-merging tasks/main.yml
CONFLICT (content): Merge conflict in tasks/main.yml
error: Failed to merge in the changes.
Patch failed at 0001 Ansible include module is deprecated in Ansible 2.4 and must be replaced with either `import_tasks` for static inclusions or `include_tasks` for dynamic inclusions. As such roles/tasks/main.yml has been updated with `include_tasks`. Please test on previous version to Ansible 2.4 before merging.
Use 'git am --show-current-patch' to see the failed patch

Resolve all conflicts manually, mark them as resolved with
"git add/rm <conflicted_files>", then run "git rebase --continue".
You can instead skip this commit: run "git rebase --skip".
To abort and get back to the state before "git rebase", run "git rebase --abort".

which means that there is some sort of CONFLICT and conflicting file is mentioned. It also says what to do to solve this conflict:

Resolve all conflicts manually, mark them as resolved with
"git add/rm <conflicted_files>", then run "git rebase --continue".

So in this state you should edit tasks/main.yml to resolve conflicts. This file will have some lines which mark what is conflicting with what. Like here in no. 5. After that it is ok to continue with adding a modified file (git add <filepath>) and finishing rebase with git rebase --continue.

Last thing to do is to push your changes to remote repository. To do that you use git push. However after rebase git push will report that it cannot complete operation and will detail why it cannot do this:

To github.com:Jaczel/ansible-role-docker.git
 ! [rejected]        ansible2.4 -> ansible2.4 (non-fast-forward)
error: failed to push some refs to 'git@github.com:Jaczel/ansible-role-docker.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Integrate the remote changes (e.g.
hint: 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details

As you need to overwrite your commit history (you are changing it by rebasing) you need to forecfully push your changes with git push -f and this would present you with nice output which finishes the job:

Counting objects: 4, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (3/3), done.
Writing objects: 100% (4/4), 1.22 KiB | 1.22 MiB/s, done.
Total 4 (delta 3), reused 1 (delta 1)
remote: Resolving deltas: 100% (3/3), completed with 3 local objects.
To github.com:Jaczel/ansible-role-docker.git
 + 8b54427...f3a7ffb ansible2.4 -> ansible2.4 (forced update)
paulfantom commented 6 years ago

This might also help with understand what rebase do: https://github.com/edx/edx-platform/wiki/How-to-Rebase-a-Pull-Request

Jaczel commented 6 years ago

Thanks again @paulfantom and sorry for the headaches.

paulfantom commented 6 years ago

@Jaczel no problem :smile:

mongrelion commented 6 years ago

Great job, guys