Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Adding tests to 02-checkrun.sh #19

Merged
merged 5 commits into from
Feb 4, 2022
Merged

Adding tests to 02-checkrun.sh #19

merged 5 commits into from
Feb 4, 2022

Conversation

LegenJCdary
Copy link
Collaborator

No description provided.

@LegenJCdary LegenJCdary force-pushed the checkrun_tests branch 13 times, most recently from b088373 to 6218d0d Compare February 3, 2022 14:23
@LegenJCdary LegenJCdary force-pushed the checkrun_tests branch 2 times, most recently from f4f9f6a to 6bf8fd5 Compare February 3, 2022 21:27
@LegenJCdary LegenJCdary force-pushed the checkrun_tests branch 6 times, most recently from 4e6b00a to 6bdeb1c Compare February 4, 2022 09:05
@LegenJCdary LegenJCdary requested a review from cinek810 February 4, 2022 09:48
@@ -4,4 +4,6 @@ infrastructures:
- name: testing
inventory: ./test_infra1_inv.yaml
- name: prod
inventory: ./prod_infra1.inv.yaml
inventory: ./prod_infra1_inv.yaml
- name: locked
Copy link
Owner

Choose a reason for hiding this comment

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

strange name for stage

config = load_configuration()
validate_option_values_with_config(config, options)
validate_option_values_against_config(config, options, required_opts)
Copy link
Owner

Choose a reason for hiding this comment

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

sounds good but it's nice to have separate commit for just name change to make the history easier to check. Maybe not so important on current stage.

@@ -129,6 +129,7 @@ def validate_options(options, subcommand):
required = ["infra", "stage"]
notsupported = ["task", "commit"]
elif subcommand == "list":
required = []
Copy link
Owner

Choose a reason for hiding this comment

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

isn't it redundant? required is defined before if

@@ -354,7 +354,7 @@ def run_task(config: dict, options: dict, inventory: str):
sys.exit(70)
else:
for playbook in playbooks:
command = ["ansible-playbook", "-l", options["infra"], "-i", inventory, playbook]
command = ["ansible-playbook", "-i", inventory, playbook]
Copy link
Owner

Choose a reason for hiding this comment

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

It's good that it's a separate commit but this is rally a fix, so commit message should mention that

@@ -0,0 +1,2 @@
420
cinek
Copy link
Owner

Choose a reason for hiding this comment

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

new line at the end of yaml? strange to call inventory "lock" :) If it's just for tests... I'd say add it to setup_hook so we can run tests locally without a file being copied prior running

- name: parent_work_dir
value: "/tmp"

- name: parent_config_dir
Copy link
Owner

Choose a reason for hiding this comment

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

this one has to be hardcoded - can't read config without that

@LegenJCdary LegenJCdary requested a review from cinek810 February 4, 2022 09:49
@cinek810 cinek810 merged commit 2399ec4 into main Feb 4, 2022
@cinek810 cinek810 deleted the checkrun_tests branch February 4, 2022 09:50
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants