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

Decision Record (ADR) for profiles.yml directory search order #5425

Closed
wants to merge 2 commits into from

Conversation

dbeatty10
Copy link
Contributor

@dbeatty10 dbeatty10 commented Jun 29, 2022

Description

This PR is for an ADR. See #5411 and the ADR itself for a full description.

Checklist

@cla-bot cla-bot bot added the cla:yes label Jun 29, 2022
@github-actions
Copy link
Contributor

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

@dbeatty10 dbeatty10 marked this pull request as ready for review June 29, 2022 21:06
@dbeatty10 dbeatty10 requested a review from a team as a code owner June 29, 2022 21:06
@emmyoop emmyoop added the Skip Changelog Skips GHA to check for changelog file label Jun 30, 2022
- property definitions for the desired target database
- secrets to plug into the definition slots

There are two main design requirements in terms of discoverability and accessibility:
Copy link
Contributor

Choose a reason for hiding this comment

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

Discoverable/Accessible in what sense? Like inside your project?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, "discoverable/accessible" was intended to correlate with being "safe to store in GitHub".


#### Approach 1

A reason given for the current order of precedence (emphasis mine):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you're right here so feel free to remove "(emphasis mine)"

1. property definitions are easily discoverable (since they are not sensitive)
1. secrets are non-discoverable and access is restricted per security policies

#### Approach 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#### Approach 1
#### Approach 1 (current approach)

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 intended to describe that both approach 1 and 2 are possible currently, and each are probably used frequently.

1 would be more likely to occur in a local environment.
2 is probably used most often in continuous integration (CI) environments.

I will re-work this section for clarity.

What I probably should have used as a guiding light instead:

  • Categorizing as Approach A or Approach B entirely depends on if any secrets are in the profiles.yml file or not.

Whether there are any secrets depends whether it can be stored in VCS or not. Presence in VCS (or not) will then affect where the file is located within the file system.

@leahwicz
Copy link
Contributor

@dbeatty10 thanks so much for making this!

I get a little lost going through this so let me list what I followed and then where it got blurry for me.

  1. Initially, you describe the current behavior for how we look for certain yml files and then you propose a new search order. - I followed
  2. Then list approaches to how to store properties and secret - I followed the approaches but got confused as to if these were current approaches people used today or if these were approaches you were proposing us to implement. I think these are current ways we can approach things today so maybe that could be clearer
  3. Options section- from my perspective, you already have a solution because you listed it as the desired outcome above so it doesn't seem like the other outcomes are valid at this point. Does that make sense? Maybe move the desired outcome section down to the Decision section?

If you want, we can meet and chat about this if that's easier and this feedback isn't very clear.

@dbeatty10
Copy link
Contributor Author

@leahwicz Your feedback is great! Thank you for outlining where it was clear and where it was unclear.

In my mind, there's five main topics to cover:

  1. Current search order
  2. Proposed search order
  3. Current options for connection properties and connection secrets (two approaches)*
  4. Listing of alternatives to the proposal (to proactively answer the question, "did you think about X instead?")
  5. Pros and cons / tradeoffs of each alternative that was seriously considered

In my attempt to cover all of those, I think it got a bit confusing. Thanks to your feedback, I will try to reorganize for readability.

Probably the most confusing part was that while explaining the pros and cons of the two ways that profiles.yml can be used, I made it sound like these were options under consideration (when in fact, that was settled long, long ago).

*I wonder if it would be worth creating a stand-alone post-hoc ADR for the two ways that profiles.yml can be used (plain-text secrets vs. exclusively environment variables rather than plain-text secrets)?

@jtcohen6
Copy link
Contributor

@dbeatty10 Is there any outstanding work here, or is it ready to be merged in? I'd love to have it in the repo, given the questions we've got from folks since the v1.3 release, which have largely been answered by the level of care and consideration you put into writing this up.

@github-actions
Copy link
Contributor

This PR has been marked as Stale because it has been open with no activity as of late. If you would like the PR to remain open, please comment on the PR or else it will be closed in 7 days.

@github-actions github-actions bot added the stale Issues that have gone stale label May 17, 2023
@github-actions
Copy link
Contributor

Although we are closing this PR as stale, it can still be reopened to continue development. Just add a comment to notify the maintainers.

@github-actions github-actions bot closed this May 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes Skip Changelog Skips GHA to check for changelog file stale Issues that have gone stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants