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

Auto-Generate config documentation #2694

Merged
merged 31 commits into from
Mar 1, 2023

Conversation

ThomasLaPiana
Copy link
Contributor

@ThomasLaPiana ThomasLaPiana commented Feb 24, 2023

Closes #2627

Code Changes

  • autogenerate the config file as valid toml that includes helpful metadata
  • add an assertion that there are no "TODO" descriptions
  • add a page for it in the docs
  • added a new generate_docs Nox session, included as part of the backend checks
  • make this the default file that gets output as part of fides init
  • copy relevant docs from the new site back here in preparation for removing config docs from the new site (will end up cross-linking)
  • overhaul how analytics consent requests work
    • refactor the check_and_update_analytics_config function so that it doesn't check for a none value (since the default is now true, this is pointless)
    • do not call the function from the main code-path, only call it from fides init or fides deploy up
    • combine the logic for consent/writing default config/updating config values into a reusable function for init and deploy up
  • add a link to the config docs page at the top of the config file

Steps to Confirm

  • spin up the docs with nox -s docs_serve and visit the configuration page. Confirm the toml snippet is rendering properly
  • Verbiage/accuracy check on the docstring updates
  • go into an empty dir and run fides init, verify that the new config file is produced
  • verify that the new config file can be used to configure a fides instance (make updates to it and run fides view config to verify the updates)
  • verify the new generat_docs nox session and CI check

Pre-Merge Checklist

Description Of Changes

This PR fundamentally changes how our "default" config file is produced and therefore also fundamentally changes fides init.

The new config file is built from the source code directly and includes with it all of the useful metadata we have as part of Pydantic and Pydantic Field objects. An important piece is that the generated config file is still valid toml and will be what the user interacts with directly when configuring their instance! CI Checks will fail if a config value doesn't have a description, the description is blank, or it is "TODO"

Additionally, the new configuration file reference is built and hosted on the docs site, as a reference to users on older versions or who have been upgrading and used a past version of fides init. The new configuration docs will be cross-linked from the new docs site.

Note that as part of this change, how/when we collect user consent has changed. It is less intrusive now. It defaults to opt-out (no dark patterns here) but gives the option to opt-in upon fides init and will update the config file directly.

Note: Given the lack of a None type in toml, Optional or empty values are no longer valid in the configuration file! If needed, you can set a default value and do a truthiness check. i.e. "" == False, as does [] == False, so we need to leverage that type of check instead.

Preview of new autogenerated config

# Fides Configuration File
# Additional Documentation at : https://ethyca.github.io/fides/stable/config/

#--------------#
#-- ADMIN_UI --#
#--------------------------------------------------------------------#
[admin_ui] # Configuration settings for the Admin UI.

# Toggle whether the Admin UI is served.
enabled = true # boolean

#---------#
#-- CLI --#
#--------------------------------------------------------------------#
[cli] # Configuration settings for the command-line appliation.

# A fully anonymized unique identifier that is automatically generated
# by the application. Used for anonymous analytics when opted-in.
analytics_id = "951ddced878629858409539c4296ad8c" # string

# When set to True, disables functionality that requires making calls
# to a Fides webserver.
local_mode = false # boolean

# The protocol used by the Fides webserver.
server_protocol = "http" # string

# The hostname of the Fides webserver.
server_host = "localhost" # string

# The port of the Fides webserver
server_port = "8080" # string

#--------------#
#-- DATABASE --#
#--------------------------------------------------------------------#
[database] # Configuration settings for the application database.

# Number of concurrent database connections Fides will use for API
# requests. Note that the pool begins with no connections, but as they
# are requested the connections are maintained and reused up to this
# limit.
api_engine_pool_size = 50 # integer
...

@ThomasLaPiana ThomasLaPiana self-assigned this Feb 24, 2023
@cypress
Copy link

cypress bot commented Feb 24, 2023

Passing run #524 ↗︎

0 3 0 0 Flakiness 0

Details:

Merge 922aca7 into 7b07d98...
Project: fides Commit: 27383c725e ℹ️
Status: Passed Duration: 00:39 💡
Started: Mar 1, 2023 1:45 AM Ended: Mar 1, 2023 1:46 AM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@codecov
Copy link

codecov bot commented Feb 24, 2023

Codecov Report

Patch coverage: 83.10% and project coverage change: +0.06 🎉

Comparison is base (7b07d98) 86.51% compared to head (922aca7) 86.58%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2694      +/-   ##
==========================================
+ Coverage   86.51%   86.58%   +0.06%     
==========================================
  Files         289      290       +1     
  Lines       16127    16210      +83     
  Branches     2044     2059      +15     
==========================================
+ Hits        13952    14035      +83     
- Misses       1791     1792       +1     
+ Partials      384      383       -1     
Impacted Files Coverage Δ
src/fides/core/config/admin_ui_settings.py 100.00% <ø> (ø)
src/fides/core/config/cli_settings.py 100.00% <ø> (ø)
src/fides/core/config/database_settings.py 100.00% <ø> (ø)
src/fides/core/config/execution_settings.py 100.00% <ø> (ø)
src/fides/core/config/helpers.py 97.64% <ø> (-0.45%) ⬇️
src/fides/core/config/logging_settings.py 100.00% <ø> (ø)
src/fides/core/config/notification_settings.py 90.00% <ø> (ø)
src/fides/cli/utils.py 67.83% <17.39%> (+1.16%) ⬆️
src/fides/cli/commands/util.py 58.66% <83.33%> (+0.77%) ⬆️
src/fides/core/config/create.py 94.94% <94.94%> (ø)
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ThomasLaPiana ThomasLaPiana marked this pull request as ready for review February 27, 2023 09:25
@ThomasLaPiana
Copy link
Contributor Author

I'm fixing up the test coverage but this is good for people to start reviewing 🙏

@ThomasLaPiana
Copy link
Contributor Author

docstrings from sean:

  • redis.decode_responses — this is a boolean value that instructs Redis to automatically decode any value fetched from the cache
  • cors_origin_regex — this is a regex string that can be used to set CORS origin allowlist patterns. e.g. http://localhost/:.* to allow any localhost port

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.

Sorry for the delays in reviewing! Spent some time working through how the opt out functionality is changing since it has snuck up on us a couple of times in the past. Added one question and a couple nits but everything checked out great for me 🙌🏽

src/fides/cli/utils.py Show resolved Hide resolved
src/fides/core/config/cli_settings.py Outdated Show resolved Hide resolved
docs/fides/docs/config/index.md Outdated Show resolved Hide resolved
Copy link
Contributor

@TheAndrewJackson TheAndrewJackson left a comment

Choose a reason for hiding this comment

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

Awesome work here. I love this change! This will help a lot with understanding how each option works in fides.toml. I just left a few small not blocking nits

src/fides/cli/utils.py Outdated Show resolved Hide resolved
src/fides/core/config/utils.py Show resolved Hide resolved
docs/fides/docs/config/index.md Show resolved Hide resolved
@ThomasLaPiana
Copy link
Contributor Author

@adamsachs I saw you made some docs updates to the config, could you help me mirror them here if they aren't already in main? (it doesn't look like it)

@ThomasLaPiana
Copy link
Contributor Author

I'm being bad and merging despite not having 100% patch coverage, but overall code cov is up and the part missing coverage is something we weren't testing before either (not an excuse, but makes it feel better?)

The missing tests are around the analytics consent messaging, which require user inputs and therefore no easy way to test

@ThomasLaPiana ThomasLaPiana merged commit 606c8d9 into main Mar 1, 2023
@ThomasLaPiana ThomasLaPiana deleted the ThomasLaPiana-autogen-config-docs branch March 1, 2023 02:09
@adamsachs
Copy link
Contributor

@adamsachs I saw you made some docs updates to the config, could you help me mirror them here if they aren't already in main? (it doesn't look like it)

@ThomasLaPiana i think we're good? this PR https://github.com/ethyca/fidesdocs/pull/52 had my changes to the docs, but i don't think what you've done here interacts with that at all? in any case, those config properties are properly annotated as pydantic Fields with a good description in database_settings.py, so i think we're all covered?

let me know if i'm missing something though.

@ThomasLaPiana
Copy link
Contributor Author

@adamsachs I saw you made some docs updates to the config, could you help me mirror them here if they aren't already in main? (it doesn't look like it)

@ThomasLaPiana i think we're good? this PR ethyca/fidesdocs#52 had my changes to the docs, but i don't think what you've done here interacts with that at all? in any case, those config properties are properly annotated as pydantic Fields with a good description in database_settings.py, so i think we're all covered?

let me know if i'm missing something though.

We good we good! My bad, thanks for replying 😄

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.

Add autogenerated configuration docs
4 participants