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

[REVIEW] - udhcp.user uci commit only new hostnames #785

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ruebenramirez
Copy link
Contributor

@ruebenramirez ruebenramirez commented Oct 8, 2024

Description of PR

Instead of always committing a provided hostname, only commit new hostnames, where the new hostname doesn't match the current hostname.

Resolves #442

Previous Behavior

Any time a hostname value was available, udhcp.user would uci commit the new hostname.

New Behavior

udhcp.user will only uci commit the hostname if it is different than the current hostname.

Tests

(need some help here)

  • test against AP hardware (monitor for uci commits)
  • provide the same hostname change to the device
  • provide a new hostname to the device

@ruebenramirez ruebenramirez changed the title udhcp.user uci commit only new hostnames [REVIEW] - udhcp.user uci commit only new hostnames Oct 8, 2024
Copy link
Member

@sarcasticadmin sarcasticadmin left a comment

Choose a reason for hiding this comment

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

@ruebenramirez I think this all makes sense, the one suggestion from @kylerisse for making a compound if seems like its worthwhile.

Regarding testing this, I can circle back with you once you have time about getting it running through the real hardware CI via gitlab.

openwrt/files/etc/udhcpc.user Outdated Show resolved Hide resolved
@ruebenramirez ruebenramirez force-pushed the only-set-new-hostnames branch 2 times, most recently from 9fd3c2a to c4c2352 Compare October 10, 2024 02:31
@ruebenramirez
Copy link
Contributor Author

ruebenramirez commented Oct 10, 2024

@ruebenramirez I think this all makes sense, the one suggestion from @kylerisse for making a compound if seems like its worthwhile.

Regarding testing this, I can circle back with you once you have time about getting it running through the real hardware CI via gitlab.

Compound conditional is in place now. I'm looking forward to learning more about the hardware testing process.

@kylerisse
Copy link
Member

kylerisse commented Oct 10, 2024

@ruebenramirez I think this all makes sense, the one suggestion from @kylerisse for making a compound if seems like its worthwhile.
Regarding testing this, I can circle back with you once you have time about getting it running through the real hardware CI via gitlab.

Compound conditional is in place now. I'm looking forward to learning more about the hardware testing process.

https://github.com/socallinuxexpo/scale-network/actions/runs/11266376404/job/31329957560?pr=785 ci choking on script @ruebenramirez . I think you just need to wrap the entire compound if in another set of [ ]
or now that I look further, maybe it's because the goldens needs to be updated also. This gets me all the time also.

Instead of always committing a provided hostname, only commit new hostnames, where
the new hostname doesn't match the current hostname.

Resolves socallinuxexpo#442
@ruebenramirez
Copy link
Contributor Author

Derp! Thanks @kylerisse! Those should all be accounted for now. If we want the extra [ ]'s I can throw them in, I just didn't see them in other compound conditionals. Maybe I need to look harder though? ;)

@kylerisse
Copy link
Member

kylerisse commented Oct 10, 2024

Derp! Thanks @kylerisse! Those should all be accounted for now. If we want the extra [ ]'s I can throw them in, I just didn't see them in other compound conditionals. Maybe I need to look harder though? ;)

Not your derp. It's a weird thing we do. In this case it was stopping at nix build -L .#checks.x86_64-linux.openwrt-golden. I believe we can add you to the repo so that your checks run automatically. @sarcasticadmin has to put in a request with TPTB. CI passes now, I think we just wait for you guys to do hardware testing. For the double [ ] that's just another case of me typing before thinking... And thank you for fixing up the commit!

@sarcasticadmin
Copy link
Member

Thanks @ruebenramirez for all the fixups and @kylerisse for your feedback. I request rueben be add to the @socallinuxexpo/tech so that should make this process and future openwrt testing easier 🙂

@ruebenramirez and I will sync up on testing this on real hardware before we merge

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.

udhcp.user unnecessary commits
3 participants