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

setup public config for zos light #3

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Omarabdul3ziz
Copy link
Contributor

- add set/unset methods to the light networker
- change reading domain configuration from kernel param to the
  persistance file
expose the function to zbus from netlightd service to other services
like noded/gateway to read the domain
@Omarabdul3ziz Omarabdul3ziz marked this pull request as draft January 29, 2025 09:44
netStub := stubs.NewNetworkerLightStub(g.cl)
config, err := netStub.LoadPublicConfig(context.Background())
baseDomain := config.Domain
if baseDomain == "" || err == nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is a bit confusing
u r calling config.domain without checking the error returned from loadPublicConfig the method returns nil in case of error which will result in an error when calling config.domain
so plz check the error first then u can call config.Domain
this line

if baseDomain == "" || err == nil {

should be

if baseDomain == "" || err != nil {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, this is wrong, it was a wip comment fixed in zoslight repo but didn't do the commit here https://github.com/threefoldtech/zoslight/blob/ff8a7a63a0c9c717a3e2ceed73112e8fbf99da83/pkg/gateway/gateway.go#L348

in case an err, config.Domain will be empty anyway. but you are right cleaner to check for the error first

if !found {
netStub := stubs.NewNetworkerLightStub(g.cl)
config, err := netStub.LoadPublicConfig(context.Background())
domain := config.Domain
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as the above comment

netStub := stubs.NewNetworkerLightStub(n.cl)
config, err := netStub.LoadPublicConfig(context.Background())

if config.Domain != "" && err == nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as first comment

return nil
}

func (n *networker) LoadPublicConfig() (pkg.PublicConfig, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

no need for this method u already calling public.loadPublicConfig without changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ashraffouda
Copy link
Collaborator

please check the comments I clicked approve instead of request changes by mistake

@Omarabdul3ziz
Copy link
Contributor Author

i suggest we close this pr and import the changes from zoslight instead. did other commits there on main

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