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

create a version that supports boto3/python3, updated requirements file #26

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

Trozz
Copy link

@Trozz Trozz commented Feb 14, 2019

added a version of the file support Python 3 and boto3, functionality is the same still

if you want I will rewrite the base file as well as submit a setup.py file to support running and installing the script as an application

@gianlucaborello
Copy link
Owner

Thanks for this. I am a little bit hesitant to merge it as is because of the significant amount of code duplication, it might create a maintainability nightmare moving forward, requiring to cherry pick each change into both files.

Can you by any chance think of a way to implement the support to Python 2 + 3 that is more idiomatic, perhaps in the same script?

This might not be possible (admittedly I've never used boto3, so I'm not aware of how much changed), so I'm just asking for my information.

Thanks again

@Trozz
Copy link
Author

Trozz commented Mar 20, 2019

well boto3 is supported under python2 so I could write in python2 that is compatible with python3.

@Trozz
Copy link
Author

Trozz commented Mar 20, 2019

@gianlucaborello this is now pep8 compliant and works in both python2 / python3 using boto3

@Trozz
Copy link
Author

Trozz commented Jun 26, 2019

@gianlucaborello any chance of merging?

@gianlucaborello
Copy link
Owner

Sorry, I've been super busy and had no time to review this. I will be able to take a look and test it after things calm down a little from my end. Thanks.

instance_id = value
else:
instance_id += '-' + value
for aws_tag in instance['Instances'][0].get('Tags', []):

Choose a reason for hiding this comment

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

This looks like a minor copy/paste error. As it is currently if you pass --tags Name, it will iterate through all instance tags and append with -, regardless of what tags were passed.

I added the following break here, and now works as expected

if aws_tag['Key'] != tag:
    continue

@rromanchuk
Copy link

Sent a PR GremlinLTD#2

Not a python guy, also unfamiliar with boto3 lib, so there's probably a better way without this branching but i'm just limiting my exposure so i dont end up in the weeds. I'm now using this in my ansible playbooks.

Here's my usage in production. It's working well with the updated version, and i removed my python2 dependency.

- name: Update ssh config
  shell: ./aws-ssh-config.py --private --tags Name --default-user ubuntu > .ssh/config
  become_user: ubuntu
  args:
    chdir: /home/ubuntu

Skip appending a tag value if it's not in the given filter
@Trozz
Copy link
Author

Trozz commented Mar 16, 2020

Sent a PR Trozz#2

Not a python guy, also unfamiliar with boto3 lib, so there's probably a better way without this branching but i'm just limiting my exposure so i dont end up in the weeds. I'm now using this in my ansible playbooks.

Here's my usage in production. It's working well with the updated version, and i removed my python2 dependency.

- name: Update ssh config
  shell: ./aws-ssh-config.py --private --tags Name --default-user ubuntu > .ssh/config
  become_user: ubuntu
  args:
    chdir: /home/ubuntu

looks good, I wonder if you would be able to generate the same thing using the Ansible dynamic inventories :-)

@Trozz
Copy link
Author

Trozz commented Apr 22, 2020

@gianlucaborello any luck getting this in

@Trozz
Copy link
Author

Trozz commented Sep 3, 2020

@gianlucaborello appreciate that you've probably had other things going on, if you would like I am happy to become a collaborator on this with you to keep development up

legoscia and others added 4 commits May 11, 2021 10:46
Tag order is non-deterministic, so instances with the same tags set
might end up not getting sequential names, e.g. 'foo-bar' and
'bar-foo' instead of 'foo-bar-1' and 'foo-bar-2'.
add newer versions of python
Add newer versions of python for automated build testing
@Trozz
Copy link
Author

Trozz commented May 16, 2021

Hi @gianlucaborello,

Hope all is well, any chance we can look to get these merged? I would be happy to take over this repo if you no-longer have the time

Trozz

@borisvezmar
Copy link

Just a little nudge to show interest in this merge.
This tool is useful and it would be very nice to have it working again

@noahdahlman
Copy link

Wanted to bump this as well, would be great to have an update

@Trozz Trozz requested a review from rromanchuk March 8, 2023 01:29
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.

7 participants