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(cmd/main.py): use correct return value #4939

Closed
wants to merge 1 commit into from

Conversation

sshedi
Copy link
Contributor

@sshedi sshedi commented Feb 22, 2024

Proposed Commit Message

fix(cmd/main.py): use correct return value

Additional Context

$ cloud-init -d modules -m init
Above command returns non zero exit status

Test Steps

root@ph4dev [ ~/cloud-init ]# cloud-init -d modules -m init
2024-02-22 19:10:07,659 - handlers.py[DEBUG]: start: modules-init: running modules for init
2024-02-22 19:10:07,659 - util.py[DEBUG]: Reading from /proc/uptime (quiet=False)
...
Cloud-init v. 23.4 running 'modules:init' at Thu, 22 Feb 2024 19:10:07 +0000. Up 26599.38 seconds.
[]

root@ph4dev [ ~/cloud-init ]# echo $?
1

Checklist

Merge type

  • Squash merge using "Proposed Commit Message"
  • Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

@sshedi
Copy link
Contributor Author

sshedi commented Feb 22, 2024

I'm not sure how to add a test for this. Any inputs on the same would be helpful. Thanks.

v1[mode]["errors"] = [str(e) for e in errors]
v1[mode]["errors"] = []
for e in errors:
v1[mode]["errors"].append(str(e))
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the purpose of this change? The new code is functionally identical to the code it replaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

errors can be an integer value here, not always a list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed there was in issue in the code change, fixed it. Thanks.

@a-dubs
Copy link
Collaborator

a-dubs commented Feb 22, 2024

Also, could you please modify your proposed commit message section so that your commit message appears as a code block like the following:

fix(cmd/main.py): use correct return value

And could you move the

$ cloud-init -d modules -m init
Above command returns non zero exit status

information in the PR description to the Additional Context section and add some more context for why this change is needed and what the change is seeking to accomplish?

Thanks!

@holmanb
Copy link
Member

holmanb commented Feb 23, 2024

@sshedi Do you use this for something? This doesn't normally run during boot, so I'm curious what you use it for.

@sshedi
Copy link
Contributor Author

sshedi commented Feb 23, 2024

@sshedi Do you use this for something? This doesn't normally run during boot, so I'm curious what you use it for.

In one of our CI/CD pipelines, I tried adding:

echo -e "\nRun cloud-init with all configured modules"
for m in init config final; do
  cmd="cloud-init --file user-data -d modules --mode $m"
  if $cmd; then
    echo -e "\nPASS: $cmd"
  else
    echo -e "\nFAIL: $cmd" 1>&2
  fi
done

And caught this issue. Nothing major, but I think the return value should be appropriate.

@sshedi
Copy link
Contributor Author

sshedi commented Feb 23, 2024

Also, could you please modify your proposed commit message section so that your commit message appears as a code block like the following:

fix(cmd/main.py): use correct return value

And could you move the

$ cloud-init -d modules -m init
Above command returns non zero exit status

information in the PR description to the Additional Context section and add some more context for why this change is needed and what the change is seeking to accomplish?

Thanks!

Thanks for the input and suggestions. Previously while raising PRs, this section was simple. I feel it's a bit complex now.
I understand the intent behind this but writing good commit message is already a hard task and filling this template is a bit confusing.

I will try.

@aciba90 aciba90 self-assigned this Feb 23, 2024
@sshedi sshedi force-pushed the retval-fix branch 4 times, most recently from f5e8011 to f43b448 Compare February 23, 2024 15:02
@aciba90 aciba90 removed their assignment Feb 23, 2024
Signed-off-by: Shreenidhi Shedi <shreenidhi.shedi@broadcom.com>
@aciba90
Copy link
Contributor

aciba90 commented Mar 6, 2024

Thanks, @sshedi, for raising this issue and proposing a fix. I am closing this one in favor of #5017, which includes a correct fix and integration tests. I hope that's okay from your side.

@aciba90 aciba90 closed this Mar 6, 2024
@sshedi
Copy link
Contributor Author

sshedi commented Mar 7, 2024

Thanks, @sshedi, for raising this issue and proposing a fix. I am closing this one in favor of #5017, which includes a correct fix and integration tests. I hope that's okay from your side.

No problem. Thanks for fixing it.

@sshedi sshedi deleted the retval-fix branch March 7, 2024 18:25
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.

None yet

4 participants