Add-on constants are not namespaced

[kke]

Due to the way the DSL is creating classes, they do not have a namespace during definition.

This means that any duplicate constants will generate a warning: already initialized constant and change the value globally.

Pharos.addon "foo" do
  PATH = "/etc/config"
end

Pharos.addon "bar" do
  PATH = "/etc/bar"
end

# warning: already initialized constant PATH
# Pharos::Addons::Foo::PATH => "/etc/bar"
# Pharos::Addons::Bar::PATH => "/etc/bar"
# PATH => "/etc/bar"

The best solution, I think, is to get rid of the addon DSL.

[SpComb]

The best solution, I think, is to get rid of the addon DSL.

Or less drastically, use methods/class instance attributes instead of consts. That would just call for something like a rubocop cop to check for const assignment outside of real class/module scopes.

[jnummelin]

The best solution, I think, is to get rid of the addon DSL.

The bad part is that the DSL has been given as THE interface to develop external addons with.

Is it not possible to dynamically also create the namespace for the add-on classes?

[kke]

So, add Rubocop as a runtime dependency and somehow run it against the addons before requireing them? 🤔

Or "something like rubocop", meaning something like ruby2ruby and analyze the sexp thing.

Probably easier to just tell the users to do class Foofoo < Pharos::Addon instead of Pharos.addon do.

[kke]

Classes get their name when you assign the class to a constant, except if doing that via const_set:

Foo = Class.new { puts "#{name}" } # => Foo
Addons.const_set(:Foo, Class.new { puts "#{name}" } # => 

Dynamic constant assignment is not something that is supposed to be done, I guess.

The anonymous classes do have their own constant namespaces that you can use like:

c = Class.new do
  self::FOO = "bar"
end

c::FOO # => bar

But this would then make the DSL mostly ruby, but have this strange quirk "if you use constants, use self::CONSTANT".

Getting rid of the DSL means that you have to change one line of your DSL:

- Pharos.addon "ingress-nginx" do
+ class IngressNginx < Pharos::Addon

(the addon name can be set via a method or inflected from the class name through some sort of underscore)

[kke]

In the light of this, I find it very strange that the https://github.com/kontena/pharos-cluster/blob/master/spec/pharos/addons/ingress_nginx_spec.rb#L47 passes on travis. It never does on my machine.

The Pharos::Addons::IngressNginx::DEFAULT_BACKEND_ARM64_IMAGE it refers to should give const missing and so it does.

The correct const in this case would be Object::DEFAULT_BACKEND_ARM64_IMAGE or just DEFAULT_BACKEND_ARM64_IMAGE and changing it to that makes the spec pass.

[kke]

It would be possible to parse the addon files, get the content between Pharos.addon and the last end, replace it with some class definition and just eval the resulting string.

This would keep the current "api" but define the addons as regular classes instead of anonymous ones.