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

Do not always create a fides.toml by default #2023

Merged
merged 4 commits into from
Dec 13, 2022

Conversation

ThomasLaPiana
Copy link
Contributor

@ThomasLaPiana ThomasLaPiana commented Dec 12, 2022

Closes #1661

Code Changes

  • remove the call to create the config from within the main CLI codepath
  • only run check_and_update_analytics_config when fides init is run

Steps to Confirm

  • head into a fresh directory and run fides, confirm no file was written and no .fides dir exists
  • run fides init and confirm that the flow works, as well as prompting about user analytics

Pre-Merge Checklist

  • All CI Pipelines Succeeded
  • Documentation Updated:
    • documentation complete, or draft/outline provided (tag docs-team to complete/review on this branch)
    • documentation issue created (tag docs-team to complete issue separately)
  • Issue Requirements are Met
  • Relevant Follow-Up Issues Created
  • Update CHANGELOG.md

Description Of Changes

The side-effect of having the fides.toml always be created whether it exists or not is a bit redundant and potentially error-prone (see the attached issue for more discussion). As such, the config file should only be created when a user specifically runs the fides init command

Due to this change though, we also needed to update how analytics_opt_out is handled, given that before we always expected the user to have a file-based configuration file to update. The behaviour is now that we assume analytics is false if there is no config file, but will prompt the user for it when they run fides init. This means we defaulting to opt-out and are no longer prompting the user every time they use the CLI to update this. I think this is no longer a major issue, as we should be relying more heavily on the user registration flow for valuable usage analytics.

@ThomasLaPiana ThomasLaPiana self-assigned this Dec 12, 2022
@ThomasLaPiana ThomasLaPiana changed the title Update __init__.py Do not always create a fides.toml by default Dec 12, 2022
@ThomasLaPiana
Copy link
Contributor Author

@NevilleS can you take a look at the logic changes here and lmk if you're comfortable with these changes?

Particularly around only doing analytics opt-ins when someone runs fides init. My thinking here is that most of that heavy lifting should actually be done by user registration now

@ThomasLaPiana
Copy link
Contributor Author

it looks like there is an error when running fides init and not choosing to opt-out of analytics

@ThomasLaPiana ThomasLaPiana marked this pull request as ready for review December 12, 2022 14:46
Copy link
Contributor

@SteveDMurphy SteveDMurphy left a comment

Choose a reason for hiding this comment

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

Tested in a local install of the branch and looks sharp! Added one question/comment earlier

Also, these are my favorite type of tongue twisters 😂

    if not no_analytics:

@ThomasLaPiana ThomasLaPiana merged commit 5cc3f25 into main Dec 13, 2022
@ThomasLaPiana ThomasLaPiana deleted the ThomasLaPiana-no-default-config-write branch December 13, 2022 04:46
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.

Support the ability for users to run fides without a .fides dir or file-based config
2 participants