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

#34 (Inventory Upload): Add output to determine inventory upload failure #35

Closed

Conversation

pvcasillasg
Copy link

Added block to handle errors in responses.

@jon-nfc jon-nfc added the type::feature type for Issues and Merge Requests. Can also be used in discussion in comments and commits label Oct 31, 2024
@jon-nfc
Copy link
Member

jon-nfc commented Oct 31, 2024

G'day @pvcasillasg,

As it's pretty late here so I'll review this in the morning. failing CI jobs will need to be fixed.

thank you for your contribution.

@jon-nfc jon-nfc self-requested a review October 31, 2024 19:01
@pvcasillasg pvcasillasg changed the title #34 - Add output to determine inventory upload failure #34 (Inventory Upload): Add output to determine inventory upload failure Oct 31, 2024
@jon-nfc jon-nfc mentioned this pull request Nov 1, 2024
Copy link
Member

@jon-nfc jon-nfc left a comment

Choose a reason for hiding this comment

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

Thanks again for your contribution. I will appologise for not having the fully repo setup, specifically CONTRIBUTING.md, however #36 has been raised to correct.

RE the failing CI Jobs, As long as the conventional commit jobs are passing then the PR "CI Job" wise is GTG.

Overall only minor styling changes required outside of the conventional commits items.

When complete please re-request review, IF the items from this CR are completed I don't foresee any issues in merging this and cutting a release.

@@ -103,23 +103,24 @@


- name: Upload inventory - {{ ansible_hostname }}
Copy link
Member

Choose a reason for hiding this comment

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

This name belongs to the task not the block.

The block will need a name however, possibly "Capture error and display trace information"

Comment on lines +106 to +107
block:
- ansible.builtin.uri:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
block:
- ansible.builtin.uri:
block:
- ansible.builtin.uri:

add two blank lines between tasks, begin and end

Comment on lines +120 to +121
{{ nfc_pb_disable_log | default(true) }}
rescue:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{{ nfc_pb_disable_log | default(true) }}
rescue:
{{ nfc_pb_disable_log | default(true) }}
rescue:

add two blank lines between tasks, begin and end

Comment on lines +121 to +122
rescue:
- debug:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
rescue:
- debug:
rescue:
- debug:

add two blank lines between tasks, begin and end

no_log: > # Contains a secret that logging shows
{{ nfc_pb_disable_log | default(true) }}
block:
- ansible.builtin.uri:
Copy link
Member

Choose a reason for hiding this comment

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

task is missing name. return the name that it used to have

no_log: > # Contains a secret that logging shows
{{ nfc_pb_disable_log | default(true) }}
rescue:
- debug:
Copy link
Member

Choose a reason for hiding this comment

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

use FQCN

- 201
no_log: > # Contains a secret that logging shows
{{ nfc_pb_disable_log | default(true) }}
rescue:
Copy link
Member

Choose a reason for hiding this comment

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

Whilst the output will only be shown on error, which meets the intent of #34, the play will now, not fail. you'll need to add a task to ensure the play fails.

@@ -103,23 +103,24 @@

Copy link
Member

Choose a reason for hiding this comment

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

We use conventional commit format (with footer) for ALL commit messages, please fix your commit message.

within the commits foote close #34.

If not known, since you have a single commit only, you can use git commit --amend to fix the commit message. Then you'll have to use git push --force to update the remote

@jon-nfc
Copy link
Member

jon-nfc commented Nov 11, 2024

@pvcasillasg,

are you going to action this PR? if so when? if not please close it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type::feature type for Issues and Merge Requests. Can also be used in discussion in comments and commits
Projects
Status: Stale
Development

Successfully merging this pull request may close these issues.

2 participants