Skip to content

Allow comments in runcmd and report failed commands correctly.#1049

Merged
TheRealFalcon merged 1 commit into
canonical:mainfrom
holmanb:main
Oct 7, 2021
Merged

Allow comments in runcmd and report failed commands correctly.#1049
TheRealFalcon merged 1 commit into
canonical:mainfrom
holmanb:main

Conversation

@holmanb
Copy link
Copy Markdown
Member

@holmanb holmanb commented Oct 6, 2021

Proposed Commit Message

summary: Allow comments in runcmd and report failed commands correctly.

A `runcmd` script may fail to parse properly, but does not mark `runcmd` as failed when that occurs.
Additionally `shellify()` fails to correctly parse scripts that contain a comment line.

Rectify both issues and add unit tests to verify correct behavior.

LP: #1853146

Test Steps

To reproduce these issues, use the following runcmd and check the logs after cloud-init runs.

runcmd:
   - echo start
   - # 
   - echo end

This PR contains four commits. The tests added in the first and third test will fail unless the second and fourth fix commits are applied. Rolling back to these commits from HEAD (git reset HEAD~3 && git reset HEAD~1) before running the new tests should demonstrate the failures. The tests should succeed from the second and fourth commit in this series.

Checklist:

  • [ TODO: Sign the CLA ] My code follows the process laid out in the documentation
  • [ x ] I have updated or added any unit tests accordingly
  • [ x ] I have updated or added any documentation accordingly

Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

Thanks Brett. This is a great first PR.

I left some comments inline, but one other thing we need to deal with here is that schema validation still complains:

2021-10-07 15:38:43,471 - schema.py[WARNING]: Invalid config:
runcmd.1: None is not valid under any of the given schemas

We need to make sure that type 'null' is acceptable. You should be able to quickly validate the userdata schema with cloud-init devel schema --system

Comment thread cloudinit/config/cc_runcmd.py Outdated
Comment thread cloudinit/tests/test_util.py Outdated
Comment thread cloudinit/util.py
@TheRealFalcon
Copy link
Copy Markdown
Contributor

Also, I technically broke the rules by giving you a review while still in Draft state. I'm guessing at this point this PR doesn't need to be Draft.

A `runcmd` script may fail to parse properly, but does not mark `runcmd` as failed when that occurs.
Additionally `shellify()` fails to correctly parse scripts that contain a comment line.

Rectify both issues and add unit tests to verify correct behavior.

LP: #1853146
@holmanb holmanb marked this pull request as ready for review October 7, 2021 19:48
@holmanb
Copy link
Copy Markdown
Member Author

holmanb commented Oct 7, 2021

Thanks Brett. This is a great first PR.

I left some comments inline, but one other thing we need to deal with here is that schema validation still complains:

2021-10-07 15:38:43,471 - schema.py[WARNING]: Invalid config:
runcmd.1: None is not valid under any of the given schemas

We need to make sure that type 'null' is acceptable. You should be able to quickly validate the userdata schema with cloud-init devel schema --system

Thanks for the review @TheRealFalcon! The latest commit addresses your comments (and it is rebased on main).

Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

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.

2 participants