Skip to content

T7626: op-mode: fix several op-mode command for syntax #4603

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

Merged
merged 2 commits into from
Jul 24, 2025

Conversation

tjjh89017
Copy link
Contributor

Change summary

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes)
  • Migration from an old Vyatta component to vyos-1x, please link to related PR inside obsoleted component
  • Other (please describe):

Related Task(s)

https://vyos.dev/T7626

Related PR(s)

How to test / Smoketest result

update container image XXX

And without error like

-vbash: syntax error near unexpected token `then'

Checklist:

  • I have read the CONTRIBUTING document
  • I have linked this PR to one or more Phabricator Task(s)
  • I have run the components SMOKETESTS if applicable
  • My commit headlines contain a valid Task id
  • My change requires a change to the documentation
  • I have updated the documentation accordingly

Copy link

github-actions bot commented Jul 13, 2025


PR title does not match the required format

Copy link

github-actions bot commented Jul 13, 2025


Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA


1 out of 2 committers have signed the CLA.
✅ (tjjh89017)[https://github.com/tjjh89017]
@dmbaturin
You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.

@tjjh89017
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

lemeshovich added a commit to vyos/vyos-cla-signatures that referenced this pull request Jul 13, 2025
@tjjh89017 tjjh89017 force-pushed the T7626 branch 2 times, most recently from bec6146 to 4d2183a Compare July 13, 2025 16:20
@sever-sever sever-sever requested a review from dmbaturin July 14, 2025 08:05
Copy link
Member

@sever-sever sever-sever left a comment

Choose a reason for hiding this comment

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

We used shell scripts for other commands 1d32420

Copy link
Member

@dmbaturin dmbaturin left a comment

Choose a reason for hiding this comment

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

Something certainly needs to be done about those commands. I'm not exactly against merging this PR, but the deal is that it already says "fix update container" but in reality fixes three unrelated commands, and there are more commands with embedded shell scripts still left, mainly in show log.

@tjjh89017 Do you want to take up fixing the rest? It's ok if not, then I can rename this PR to something generic and approve it.

@tjjh89017
Copy link
Contributor Author

Hi @dmbaturin
i think first thing is deciding the style for the command here for the future.
If this style is acceptable currently, I think it's still hard to find all command to fix them at once.
You can rename this PR to something generic.
Thank you

@dmbaturin
Copy link
Member

@tjjh89017 I think it's an acceptable stop-gap measure and it's good that we can identify all commands like that by grepping for sh -c.

I think the long-term plan should be to provide a function in the standardized op mode API for this, and a corresponding tag in the op mode schema — or perhaps make it implicit in the script if that function exists. I need to think about it and put those thoughts in a Phorge task, but we still need to fix the immediate issue, since converting all those commands to standardized op mode will take quite a bit longer.

@tjjh89017
Copy link
Contributor Author

@dmbaturin
Ok, I think we could consider to change the name for this PR at this moment.
and find some way to grep all of incorrect existing command to a list for new PR.
Especially, some commands require shell trick to escape the variables.

@tjjh89017 tjjh89017 changed the title T7626: op-mode: fix update container image command T7626: op-mode: fix several op-mode command for syntax Jul 16, 2025
@sever-sever
Copy link
Member

@tjjh89017 could you rebase the PR?

update container image <container name>
show environment sensors
show log certbot

Signed-off-by: Date Huang <[email protected]>
@tjjh89017
Copy link
Contributor Author

@tjjh89017 could you rebase the PR?

I rebased it.
Please help me to check (I'll wait for CI.)
Thank you

@sever-sever sever-sever requested a review from Copilot July 21, 2025 09:48
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes syntax errors in op-mode commands by properly wrapping shell conditionals in bash -c blocks. The changes address shell syntax issues that were causing errors like "syntax error near unexpected token 'then'" when executing these commands.

  • Wraps if-then-else statements in bash -c blocks to ensure proper shell execution
  • Adds proper quoting for variables and string literals within the bash commands
  • Fixes three op-mode commands: show log certbot, show environment sensors, and update container image

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
op-mode-definitions/show-log.xml.in Wraps certbot log command in bash -c for proper if-then-else execution
op-mode-definitions/show-environment.xml.in Wraps sensors command in bash -c and fixes path variable quoting
op-mode-definitions/container.xml.in Wraps container update command in bash -c and adds proper variable quoting

Copy link
Member

@dmbaturin dmbaturin left a comment

Choose a reason for hiding this comment

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

Let's merge it as is and then we can fix the remaining commands ourselves. Thanks, @tjjh89017 !

@dmbaturin dmbaturin merged commit 3882751 into vyos:current Jul 24, 2025
7 of 9 checks passed
@vyosbot vyosbot added the mirror-initiated This PR initiated for mirror sync workflow label Jul 24, 2025
@tjjh89017
Copy link
Contributor Author

thank you

@vyosbot vyosbot added mirror-completed and removed mirror-initiated This PR initiated for mirror sync workflow labels Jul 24, 2025
Copy link

CI integration 👍 passed!

Details

CI logs

  • CLI Smoketests (no interfaces) 👍 passed
  • CLI Smoketests (interfaces only) 👍 passed
  • Config tests 👍 passed
  • RAID1 tests 👍 passed
  • TPM tests 👍 passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants