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

features: add feature test methods #6181

Closed
wants to merge 6 commits into from
Closed

Conversation

zyga
Copy link
Collaborator

@zyga zyga commented Nov 21, 2018

features: add module for querying enabled snapd features

This builds upon the ability to export enabled experimental features to
userspace. We have the Go side for controlling features via "snap set
core experimental.foo 1" and the C side for querying them but we still
need the Go counterpart. This patch adds one.

The API is extremely simple, consisting of a new type, SnapdFeature,
along with a IsEnabled method that performs a stat call.

Signed-off-by: Zygmunt Krynicki me@zygoon.pl

The features directory is designed to store marker files for optional
features that are activated in snapd that need to be checked by
snap-update-ns or other programs that cannot read the state file
directly.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
This builds upon the ability to export enabled experimental features to
userspace. We have the Go side for controlling features via "snap set
core experimental.foo 1" and the C side for querying them but we still
need the Go counterpart. This patch adds one.

The API is extremely simple, consisting of a new type, SnapdFeature,
along with a IsEnabled method that performs a stat call.

Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

This looks fine but I would like to see a slightly bigger PR that uses features in settings as well because as it is right now the duplication of strings and things makes me uneasy. Or just a pointer to the follow-up with the changes?

features/features.go Outdated Show resolved Hide resolved

// String returns the name of a snapd feature.
func (f SnapdFeature) String() string {
// The constants here must be synchronized with cmd/libsnap-confine-private/feature.c
Copy link
Contributor

Choose a reason for hiding this comment

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

And they also need to be synced with overlord/configstate/configcore/experimental.go - this makes me a bit uneasy. I wonder if there is a way to sync configcore and this closer. Right now we when adding a experimental flag we need to add things in three places AFAICT, I wonder if there is a way to share the GO bits more (and maybe even the C bits but that is of course way harder).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As I said in the other PR I will follow up with using those constants there. We will only need to do two places (C and Go) which is IMO good enough for now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

we are growing a bit too many small packages I fear. If we have this the code in configcore/experimental.go should probably become dynamic and use listing here that should flag a feature as experimental and whether it's turned to default.

features/features.go Outdated Show resolved Hide resolved
The feature flag will be added with a dedicated patch.

Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
The presence or absence of the control file determines if a given
feature is active or not. Since snapd needs to manage those files it's
better to have one canonical definition.

Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>

// String returns the name of a snapd feature.
func (f SnapdFeature) String() string {
// The constants here must be synchronized with cmd/libsnap-confine-private/feature.c
Copy link
Collaborator

Choose a reason for hiding this comment

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

we are growing a bit too many small packages I fear. If we have this the code in configcore/experimental.go should probably become dynamic and use listing here that should flag a feature as experimental and whether it's turned to default.

@zyga
Copy link
Collaborator Author

zyga commented Nov 22, 2018

@pedronis this module needs to be imported from things outside of snapd. The changes to use it in configcore/experimenta.go are coming in another PR (6170 after this one lands).

@pedronis
Copy link
Collaborator

no, changes to experimental.go needs to happen together with this here

@pedronis
Copy link
Collaborator

my point is that if we have this, just using the constant is not enough, we need to have the full set of facts around features/experimental features here, and other places use the facts. I shouldn't need to go over there to know if a feature is experimental and whether is now default or not

@zyga
Copy link
Collaborator Author

zyga commented Nov 22, 2018

Closing to integrate with #6170

@zyga
Copy link
Collaborator Author

zyga commented Nov 22, 2018

This is now proposed, unified, as #6190

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants