Skip to content

ddns-scripts: improve updater help and legacy syntax error#29189

Open
dhrm1k wants to merge 1 commit intoopenwrt:openwrt-24.10from
dhrm1k:ddns-updater-legacy-syntax-hint-24.10
Open

ddns-scripts: improve updater help and legacy syntax error#29189
dhrm1k wants to merge 1 commit intoopenwrt:openwrt-24.10from
dhrm1k:ddns-updater-legacy-syntax-hint-24.10

Conversation

@dhrm1k
Copy link
Copy Markdown

@dhrm1k dhrm1k commented Apr 19, 2026

Fix updater help/error text around legacy invocation and option naming.

Ref: #27737

📦 Package Details

Maintainer: @feckert

Description:
This updates dynamic_dns_updater.sh messaging only:

  • correct -N to -n in help/error text
  • correct to much commands to too many commands
  • add a clearer hint when legacy SECTION VERBOSE style is used (for example opc 0), pointing to -S <section> -v <level> -- start

CC @feckert for review


🧪 Run Testing Details

  • OpenWrt Version: 24.10.4
  • OpenWrt Target/Subtarget: x86/64
  • OpenWrt Device: Generic (x86_64 VM)

✅ Formalities

  • I have reviewed the CONTRIBUTING.md file for detailed contributing guidelines.

If your PR contains a patch:

  • It can be applied using git am
  • It has been refreshed to avoid offsets, fuzzes, etc., using
    make package/<your-package>/refresh V=s

@github-actions github-actions Bot added the OpenWrt 24.10 Issue/PR on branch 24.10 label Apr 19, 2026
@dhrm1k dhrm1k force-pushed the ddns-updater-legacy-syntax-hint-24.10 branch from d56c9b9 to 80a9057 Compare April 19, 2026 10:13
Copy link
Copy Markdown
Member

@feckert feckert left a comment

Choose a reason for hiding this comment

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

That should be first merged to the master branch. After that we can backport this to the stable branches openwrt-24.10 and openwrt-25.12.

Please update the commit message with more context. Is the -N option always wrong? That’s never worked before? What about scripts that already use this option?

Additional make sure you do not exceed the character limit of 75 characters per line.

Use Fixes: <number> so the issue will be auto closed after merge.

Clarify why this change updates help/error text around the option name.
The issue is not that -N is always wrong. The current parser behavior
expects the lower-case form here, so showing -N in these messages is
misleading and suggests a form that does not work in this code path.

Also improve grammar in the multi-command error and add a focused hint
for legacy '<section> <level>' usage so existing scripts are easier to
adjust.

This is message-level guidance only; no runtime parsing behavior changes.

Fixes: openwrt#27737
@dhrm1k dhrm1k force-pushed the ddns-updater-legacy-syntax-hint-24.10 branch from 80a9057 to ac05f71 Compare April 20, 2026 19:20
@dhrm1k dhrm1k changed the base branch from openwrt-24.10 to master April 20, 2026 19:26
@dhrm1k dhrm1k changed the base branch from master to openwrt-24.10 April 20, 2026 19:26
@dhrm1k
Copy link
Copy Markdown
Author

dhrm1k commented Apr 20, 2026

That should be first merged to the master branch. After that we can backport this to the stable branches openwrt-24.10 and openwrt-25.12.

Please update the commit message with more context. Is the -N option always wrong? That’s never worked before? What about scripts that already use this option?

Additional make sure you do not exceed the character limit of 75 characters per line.

Use Fixes: <number> so the issue will be auto closed after merge.

Thanks for the review. I updated the commit message with additional context:

  • clarified that -N is not universally wrong, but misleading in this code path
  • mentioned impact/expectation for existing scripts
  • switched to Fixes: #27737
  • wrapped lines to <=75 chars

I force-pushed these changes to this PR branch.
If you prefer master-first, I can respin/rebase this PR to target master
(or open a master-targeted PR and link it), then backport to stable branches.
Please confirm your preferred flow.

@dhrm1k dhrm1k requested a review from feckert April 20, 2026 19:32
@feckert
Copy link
Copy Markdown
Member

feckert commented Apr 21, 2026

Ideally, I’d like to have this in the master branch so I can backport it.
But in the master branch, we’ve switched the handling to procd, so '-S $1 -v $2 -- start' is no longer supported or should be used.

@dhrm1k
Copy link
Copy Markdown
Author

dhrm1k commented Apr 21, 2026

Thanks for the clarification.

I am preparing a master-specific version aligned with the procd-based flow. It only fixes incorrect -N references in updater help and error text and does not suggest -S $1 -v $2 -- start and does not add the
-S ... -- start legacy hint.

I will open the master PR first and link it here for backport follow-up.

Fixes: #27737

@dhrm1k
Copy link
Copy Markdown
Author

dhrm1k commented Apr 21, 2026

I opened a master-first PR which matches with procd handling: #29208

After this is merged, I will submit the stable backports.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants