Skip to content
This repository was archived by the owner on Nov 30, 2022. It is now read-only.

Conversation

@daveqnet
Copy link
Contributor

@daveqnet daveqnet commented Jul 1, 2022

Purpose

I was following the steps here and had to make a few changes to get it working. I suspect this doc is just a little out of date.

Changes

  • Removed "Install fidesops in our test app" section. This is done for you already by fidesdemo.
  • Updated the ports for the Flaskr app and fidesdemo

Checklist

  • Update CHANGELOG.md file
    • Merge in main so the most recent CHANGELOG.md file is being appended to
    • Add description within the Unreleased section in an appropriate category. Add a new category from the list at the top of the file if the needed one isn't already there.
    • Add a link to this PR at the end of the description with the PR number as the text. example: #1
  • Applicable documentation updated (guides, quickstart, postman collections, tutorial, fidesdemo, database diagram.
  • If docs updated (select one):
    • 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)
  • Good unit test/integration test coverage
  • This PR contains a DB migration. If checked, the reviewer should confirm with the author that the down_revision correctly references the previous migration before merging
  • The Run Unsafe PR Checks label has been applied, and checks have passed, if this PR touches any external services

Ticket

n/a

@daveqnet daveqnet added the documentation Improvements or additions to documentation label Jul 1, 2022
@pattisdr
Copy link
Contributor

pattisdr commented Jul 1, 2022

hm, so the tutorial used to have the user check out an older branch that didn't have fidesops installed yet, to show the user how to do it. It looks like that was changed a week ago #715, so we now checkout main instead. Was this an unintended consequence @conceptualshark? I thought it was informative to show the user how fidesops was added to the docker-compose file.

@conceptualshark
Copy link
Contributor

Thank you for the catch! Yes, @pattisdr, just missed this in the change - the old branch was out of date, but I do think adding fidesops to the app is useful for the (Coming Soon™️) tutorial update.

@pattisdr
Copy link
Contributor

pattisdr commented Jul 5, 2022

sounds good @conceptualshark

@seanpreston seanpreston self-assigned this Jul 8, 2022
@daveqnet
Copy link
Contributor Author

What are you thinking on this one, folks: should I close this PR and you handle it as Issue #258 instead?

@conceptualshark
Copy link
Contributor

@daveqnet I think it's worth doing for now - it'll take me a while to get to the full tutorial overhaul, and I'd rather have it working!

@daveqnet
Copy link
Contributor Author

@daveqnet I think it's worth doing for now - it'll take me a while to get to the full tutorial overhaul, and I'd rather have it working!

Thanks @conceptualshark, sounds like a plan. This PR got a bit stale since I created it, so I've gone ahead and resolved the conflicts with main. It should be ready to merge now.

@conceptualshark conceptualshark merged commit e6788c7 into main Jul 22, 2022
@conceptualshark conceptualshark deleted the daveqnet-fidesopstutorial-1 branch July 22, 2022 13:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants