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

Fix upstream issues #1

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

Fix upstream issues #1

wants to merge 7 commits into from

Conversation

gkranis
Copy link

@gkranis gkranis commented Apr 12, 2023

Fixes the following upstream issues, one commit per issue (i am not opening an upstream PR yet, because I am most probably breaking the tests)

ahelal#69
trivial

ahelal#68
trivial fix however the output is logged at the end instead of streamed, so it could be improved. trivial now streaming output

ahelal#70

ansiblepush encodes the inventory data for host and groups in yaml files (stores them in a static directory, same for all suites), then invokes ansible with a dynamic inventory script that parses them. As explained in the issue using the same dir causes conflicts.

I changed the behavior in that the yaml files are stored in a unique per kitchen suite subdirectory. The dynamic inventory script then reads the ENV var INSTANCE_NAME to know which subdir to read the yaml files from

ahelal#71
trivial, but only possible because we fixed the previous issue

@gkranis gkranis changed the title Fixes Fix upstream issues Apr 12, 2023
@gkranis gkranis requested a review from jamie-staib April 12, 2023 14:16
@gkranis
Copy link
Author

gkranis commented Apr 24, 2023

One more thing. I prefix ansible's logs with the kitchen suite's name. Thats not an upstream issue, its a quick workaround for test-kitchen/test-kitchen#583 . While normally we dont need it since jenkins exports the kitchen logs per suite, we need it in exceptional cases where CI runs die before this happens

@jamie-staib
Copy link

@gkranis Adding @BaCaRoZzo as a reviewer instead of myself, as Ruby is totally out of my knowledge scope.

@jamie-staib jamie-staib requested review from BaCaRoZzo and removed request for jamie-staib April 26, 2023 14:08
Copy link

@BaCaRoZzo BaCaRoZzo left a comment

Choose a reason for hiding this comment

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

Took me a bit, sorry. Had to look up the API online. 😅

Looks good, for what I can tell. Thanks for splitting it into meaningful smaller commits, that helped a lot.

@@ -18,6 +18,7 @@ def generate_instance_inventory(name, host, mygroup, instance_connection_option,
end

temp_hash = {}
temp_hash['ansible_host'] = host
temp_hash['ansible_ssh_host'] = host

Choose a reason for hiding this comment

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

From a quick check, this doesn't seem to be used any more.
If this is not used, can't we remove it? Or are you going the most conservative route just in case? 🤔

Copy link
Author

Choose a reason for hiding this comment

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

@BaCaRoZzo I seem to remember that I removed it and something broke so i left it as is, but im not 100% sure

Copy link
Author

Choose a reason for hiding this comment

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

EIther that or I thought i might want to keep the plugin's compatibility with older ansible versions

@gkranis
Copy link
Author

gkranis commented Jun 2, 2023

Fixed the tests and sent a copy of that PR upstream, maybe we are lucky and we dont have to maintain this

@gkranis
Copy link
Author

gkranis commented Jun 2, 2023

ahelal#72

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.

3 participants