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

Migrate ec2 instance modules #354

Merged
merged 1 commit into from
May 13, 2021

Conversation

jillr
Copy link
Collaborator

@jillr jillr commented May 3, 2021

SUMMARY

Moves ec2_instance and ec2_instance_info from community.aws to amazon.aws via git patch.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

ec2_instance
ec2_instance_info

ADDITIONAL INFORMATION

This is the first time we're doing this, git history should be preserved but Github may want to squash. I don't love the idea of directly committing to origin to get around that. ETA: I should be able to do a manual merge from the cli though?

Migration was done via:

COLLECTIONS_PATH/community/aws$ git log --pretty=email --patch-with-stat --reverse --full-index --binary -- tests/integration/targets/ec2_instance/ plugins/modules/ec2_instance.py plugins/modules/ec2_instance_info.py > /tmp/patch`

COLLECTIONS_PATH/amazon/aws$ git am < /tmp/patch

then adding the _facts.py symlink and sed'ing the FQCNs.
A PR will follow to remove the modules from community.aws after this has merged.

@tremble
Copy link
Contributor

tremble commented May 4, 2021

recheck

Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

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

A couple of nits. I also don't see an update to the module_defaults.

If at all possible I would rather see this land including the history. To me the history is extremely valuable and I have missed on multiple occasions having the original ansible/ansible history available to me (without making multiple queries)

changelogs/fragments/migrate_ec2_instance.yml Outdated Show resolved Hide resolved
tests/integration/targets/ec2_instance/aliases Outdated Show resolved Hide resolved
@jillr jillr force-pushed the migrate_ec2_instance branch 3 times, most recently from 906cbe3 to 32cc313 Compare May 4, 2021 22:55
@jillr jillr requested a review from tremble May 6, 2021 17:37
@jillr jillr force-pushed the migrate_ec2_instance branch 4 times, most recently from 209feca to dbdf374 Compare May 6, 2021 23:33
@tremble
Copy link
Contributor

tremble commented May 7, 2021

@goneri Looks like the tests didn't trigger again

@@ -0,0 +1,5 @@
dependencies:
- prepare_tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

prepare_tests includes an empty file. Why do we need it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TBH that's been migrated around as-is since it was in ansible/ansible. It would probably we good to do a general review but I'd prefer not to make too many changes here beyond what's needed to get the modules working

@jillr jillr force-pushed the migrate_ec2_instance branch from dbdf374 to 18679c7 Compare May 7, 2021 16:44
@jillr jillr requested a review from alinabuzachis May 7, 2021 17:14
@jillr
Copy link
Collaborator Author

jillr commented May 10, 2021

I"m going to try a cli merge. If anything goes wrong (ie, history is squashed) we can revert. (after a fresh rebase onto main)

@jillr jillr force-pushed the migrate_ec2_instance branch from 18679c7 to b43e12e Compare May 10, 2021 16:51
Copy link
Collaborator

@alinabuzachis alinabuzachis left a comment

Choose a reason for hiding this comment

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

LGTM (the integration tests that are skipped).

@jillr
Copy link
Collaborator Author

jillr commented May 10, 2021

The git cli won't actually work for this, it wants to either squash or merge. After discussing with @gravesm and @goneri our options here are:

  1. Squash. In this case we lose all the git history from community.aws - this means folks need to look at up to 3 repos (amazon.aws, community.aws, ansible/ansible) to trace the history of changes. Very not ideal.
  2. Merge commit. None of us like this option as it makes git log messy.
  3. Reproduce the git patching process that was done on this branch, and that we approve of the resulting effect of, directly onto main (in my local checkout) and push that to origin (ansible-collections/amazon.aws/tree/main). Then update/rebase this PR so that it should just include the docs updates and the 's/community.aws/amazon.aws/' pieces. We also don't love this option, but it seems to be the one that leaves us with the cleanest git history.

Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

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

FWIW I'd probably go with option 3:

  • Keeps the history
  • Results in a single additional commit with the things specifically related to the migration.

@jillr
Copy link
Collaborator Author

jillr commented May 13, 2021

Patched the files from community onto main (eek!) will cleanup the conflicts here and see how it goes

Moves ec2_instance and ec2_instance_info from community.aws to amazon.aws
via git patch.
@jillr jillr force-pushed the migrate_ec2_instance branch from b43e12e to 2f15959 Compare May 13, 2021 17:23
@jillr jillr added the gate label May 13, 2021
@ansible-zuul ansible-zuul bot merged commit 9c42b6c into ansible-collections:main May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants