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

[FEATURE] Add Provider for domain_code #55

Closed
wants to merge 2 commits into from
Closed

Conversation

margau
Copy link
Contributor

@margau margau commented Feb 21, 2020

This PR adds a provider for domain_code, which allows to display the domain of the gateway inside meshviewer etc.
Example usage: https://map.ffm.freifunk.net/#!/de/map/aaff04000802

@rubo77
Copy link
Contributor

rubo77 commented Feb 23, 2020

Sieht gut aus, ist einfacher als wenn man bei jedem Gateway einzeln die Domain in den alias.json eintragen muss

Copy link
Member

@TobleMiner TobleMiner left a comment

Choose a reason for hiding this comment

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

Overall good. Necessary changes are small, thus I'll implement them myself.

@@ -38,6 +38,7 @@ def handle(self):
# Clone global environment and populate with interface-specific data
local_env = dict(env)
local_env['batadv_dev'] = batadv_dev
local_env['domain_code'] = domain_code
Copy link
Member

Choose a reason for hiding this comment

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

Why do we put that in the local_env? Isn't that a global setting? Think this should go here:

get_handler(get_providers(args.directory), batadv_ifaces, batadv_mesh_ipv4_overrides, {'mesh_ipv4': args.mesh_ipv4})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I probably misunderstood how the local_env is used exactly.

Copy link
Member

Choose a reason for hiding this comment

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

Well, local_env is intended for values that can change per request. In contrast the global environment - which was hard to spot since it is only passed as a function argument - contains values that are fixed, no matter what interface a request is received on.

@@ -78,6 +79,9 @@ def handle(self):
parser.add_argument('-m', dest='mesh_ipv4',
metavar='<mesh_ipv4>',
help='mesh ipv4 address')
parser.add_argument('-n', dest='domain_code',
metavar='domain_code',
help='Domain Code for system/domain_code')
Copy link
Member

Choose a reason for hiding this comment

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

There should be a reasonable default for this value. I'd suggest setting it to 'default' when -n is not specified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need a default? If nothing is set, there should be no domain transmitted (as it is now)?

Copy link
Member

Choose a reason for hiding this comment

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

Hm, ok. That is a side effect of having a None value in there then. I would have expected it to break due to an exception somewhere in handler processing when a handler returns None.

I'll add an is_enabled method to data sources so they can signal wether they should be called or not. This seems a lot clearer to me than handling None values all the time.

@TobleMiner
Copy link
Member

I'd suggest this solution: https://github.com/ffnord/mesh-announce/compare/master...TobleMiner:feature-domain-name?expand=1

Please test and leave feedback.

@margau
Copy link
Contributor Author

margau commented Feb 26, 2020

The "is_enabled" flag seems to be broken at the moment:

  File "/opt/mesh-announce/providers/__init__.py", line 145, in call  
    if source.is_enabled(env):  
AttributeError: 'Source' object has no attribute 'is_enabled'```

Thanks for the improvements!

@TobleMiner
Copy link
Member

Ah, my fault. Need to proxy the call through the Source object. Should be fixed now. Please try again.

@margau
Copy link
Contributor Author

margau commented Feb 26, 2020

Test without and with the Gateway-Option was successfull, everything okay from my side.

@TobleMiner
Copy link
Member

Just tested it as well. Seems to work fine.

@TobleMiner
Copy link
Member

Merged, thanks for the PR!

@TobleMiner TobleMiner closed this Feb 26, 2020
@herbetom
Copy link
Contributor

Is there an option to set the domain_code without starting multiple instances (in a multi-domain Setup)?

From the README.md this seems to be the way to start mesh-announce for a multi-domain site: respondd.py -d /opt/mesh-announce/providers -i meshvpn-one -i br-one -i bat-one -b bat-one -i meshvpn-two -i br-two -i bat-two -b bat-two -i meshvpn-three -i br-three -i bat-three -b bat-three

As far as i can see, it is only possible to specify a single -n <domain code>?

@TobleMiner
Copy link
Member

Hm, right. That is kind of required. However currently it is not possible.

It's actually an option that would probably need to be attached to the -i option again. Please make a separate issue for that. Makes it easier to track it.

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.

None yet

4 participants