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

(PDK-1339) Read or interview for analytics config #657

Merged
merged 4 commits into from
May 14, 2019

Conversation

rodjek
Copy link
Contributor

@rodjek rodjek commented Apr 11, 2019

On the first run of PDK if the vendor-wide analytics config file is absent ($XDG_CONFIG_HOME/puppetlabs/analytics.yml or %LOCALAPPDATA%/puppetlabs/analytics.yml) the user will be greeted with the following question.

pdk (INFO): PDK collects anonymous usage information to help us understand how it is being used and make decisions on how to improve it. You can find out more about what data we collect and how it is used in the PDK documentation at https://puppet.com/docs/pdk/.
[Q 1/1] Do you consent to the collection of anonymous PDK usage information?
--> Yes

pdk (INFO): You can opt in or out of the usage data collection at any time by editing the analytics configuration file at /home/tsharpe/.config/puppetlabs/analytics.yml and changing the 'disabled' value.

If PDK is not being run interactively (i.e. being called from a script) then the interview will be skipped and the user will be automatically opted out of analytics.

@coveralls
Copy link

coveralls commented Apr 11, 2019

Coverage Status

Coverage increased (+0.3%) to 93.328% when pulling 072dfb8 on rodjek:pdk-1339 into d3db714 on puppetlabs:master.

@scotje
Copy link
Contributor

scotje commented Apr 16, 2019

Overall I think this is looking great @rodjek ! I think it makes a great framework to initially provide a consistent interface over a bunch of disparate config files while also letting us slowly unify/replace those config files when and where it makes sense.

I know this is still WIP so I wanted to check if it was already in your plans to have PDK read the values from the Bolt config if present on first run, but then write them out to the vendor config location so that we don't lose that config if the user later uninstalls Bolt and/or deletes their Bolt config dir?

Also where would you envision hooking in a prompt if we wanted to interactively ask them if they want to opt-out absent any existing config?

@rodjek
Copy link
Contributor Author

rodjek commented Apr 16, 2019

I know this is still WIP so I wanted to check if it was already in your plans to have PDK read the values from the Bolt config if present on first run, but then write them out to the vendor config location so that we don't lose that config if the user later uninstalls Bolt and/or deletes their Bolt config dir?

Yep, it all happens in where the user.analytics configuration is defined. If we can't load the values from the mounted vendor-wide config, it'll default to the value from the bolt configuration (if it exists). eg https://github.com/puppetlabs/pdk/pull/657/files#diff-062b30bfb8a3ec0cbbb1df8a3e3b1df6R20

When the default_to blocks are executed, they automatically trigger the configuration to be written to disk after the value is resolved, so we basically get the config migration for free.

Also where would you envision hooking in a prompt if we wanted to interactively ask them if they want to opt-out absent any existing config?

My plan is to check for the presence of the vendor-wide config file when the CLI is instantiated but before the analytics client is instantiated. If the file doesn't exist, we'll inform the user and give them a link to the analytics documentation. It won't be an interactive prompt though, partly because that's not what's in the ticket but mostly because that would interfere with any automated --force workflows that users might have.

@scotje
Copy link
Contributor

scotje commented Apr 16, 2019

My plan is to check for the presence of the vendor-wide config file when the CLI is instantiated but before the analytics client is instantiated. If the file doesn't exist, we'll inform the user and give them a link to the analytics documentation. It won't be an interactive prompt though, partly because that's not what's in the ticket but mostly because that would interfere with any automated --force workflows that users might have.

OK, we should discuss this with @turbodog more because I think he was hoping we could have a prompt. (I think it would be reasonable to bypass the prompt on any --force invocations.)

@turbodog
Copy link
Contributor

Yeah I agree that we should go with the prompt route. If --force is present and there's no setting set then maybe print a warning?

@rodjek rodjek force-pushed the pdk-1339 branch 12 times, most recently from d592c47 to bff7309 Compare May 9, 2019 11:22
@rodjek rodjek changed the title [WIP] (PDK-1339) (PDK-1339) Read or interview for analytics config May 9, 2019
@jbondpdx
Copy link
Contributor

jbondpdx commented May 9, 2019

@rodjek , the text looks good to me! Let's make the URL https://puppet.com/docs/pdk/latest/pdk_install.html.

@rodjek
Copy link
Contributor Author

rodjek commented May 13, 2019

Thanks @jbondpdx, I've updated the URL!

Copy link
Contributor

@scotje scotje left a comment

Choose a reason for hiding this comment

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

Looks good to me! I left a few small comments but nothing that should block merging.

Nowhere is actually instantiating an analytics client yet correct? So I shouldn't expect to see the debug messages yet?

spec/spec_helper.rb Show resolved Hide resolved
post_message = _(
'You can opt in or out of the usage data collection at any time by ' \
'editing the analytics configuration file at %{path} and changing ' \
"the '%{key}' value.",
Copy link
Contributor

Choose a reason for hiding this comment

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

We should maybe pre-word wrap these at 80 characters or something. Right now the terminal will wrap in the middle of a word. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also an extra newline after the "post_message" would help it to not get lost in the results of the actual command they ran.

Copy link
Contributor Author

@rodjek rodjek May 14, 2019

Choose a reason for hiding this comment

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

I've updated the logger formatter so that we can optionally word wrap the output (and indent the wrapped lines for readability) which gives us the following:

pdk (INFO): PDK collects anonymous usage information to help us understand how
            it is being used and make decisions on how to improve it. You can
            find out more about what data we collect and how it is used in the
            PDK documentation at
            https://puppet.com/docs/pdk/latest/pdk_install.html.

[Q 1/1] Do you consent to the collection of anonymous PDK usage information?
--> Yes

pdk (INFO): You can opt in or out of the usage data collection at any time by
            editing the analytics configuration file at
            /home/tsharpe/.config/puppet/analytics.yml and changing the
            'disabled' value.

end

def self.analytics_config_path
ENV['PDK_ANALYTICS_CONFIG'] || File.join(File.dirname(PDK::Util.configdir), 'puppetlabs', 'analytics.yml')
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we take this opportunity to rename this to ~/.config/puppet/analytics.yml or are there other things already using ~/.config/puppetlabs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm 👍 to using puppet instead of puppetlabs

@rodjek rodjek merged commit 2f83c0f into puppetlabs:master May 14, 2019
@rodjek rodjek deleted the pdk-1339 branch May 14, 2019 05:10
@smortex
Copy link

smortex commented Jun 28, 2019

If PDK is not being run interactively (i.e. being called from a script) then the interview will be skipped and the user will be automatically opted out of analytics.

This seems to not work as expected on Travis-CI:
https://travis-ci.com/smortex/puppet-riemann/jobs/211705608

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

Successfully merging this pull request may close these issues.

7 participants