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

Add docs for multi repo pipelines #1142

Merged
merged 8 commits into from
Sep 29, 2023
Merged

Conversation

ellisonc
Copy link

No description provided.

@netlify
Copy link

netlify bot commented Sep 26, 2023

Deploy Preview for pensive-meitner-faaeee ready!

Name Link
🔨 Latest commit 6a886b3
🔍 Latest deploy log https://app.netlify.com/sites/pensive-meitner-faaeee/deploys/6516d10e4d0ad30008db8ee8
😎 Deploy Preview https://deploy-preview-1142--pensive-meitner-faaeee.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

_docs-sources/pipelines/multi-account/index.md Outdated Show resolved Hide resolved
_docs-sources/pipelines/multi-account/index.md Outdated Show resolved Hide resolved
_docs-sources/pipelines/multi-account/index.md Outdated Show resolved Hide resolved
sidebars/pipelines.js Outdated Show resolved Hide resolved
MoonMoon1919
MoonMoon1919 previously approved these changes Sep 27, 2023
Copy link
Contributor

@MoonMoon1919 MoonMoon1919 left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@josh-padnick josh-padnick left a comment

Choose a reason for hiding this comment

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

It's pretty cool to see this laid out, and we get our first instance of configuring something! I left some comments on where to put the content, adding more context on when this would make sense, and on the config file itself.

The one other helpful thing would be to write more about the security implications of adding another repo. I guess that means that whoever controls the main branch of that repo can execute with the full AWS permissions granted in this repo, so we should probably give a warning or overview of what kind of trust assertions you need to be able to make before you can responsibly add another infra-live repo.

label: "Multiple Infrastructure-Live Repos",
type: "doc",
id: "pipelines/multi-account/index",
},
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels like it goes beyond the scope of "getting started." Should we introduce a new section called "Configuration" and put it in there? Perhaps we could also move the following content to that section:

  • The GitHub Enterprise Users section of the "Using Pipelines" page (that could go under its own page called "GitHub Enterprise")
  • Usage data
  • Repository access (maybe?)

Copy link
Author

Choose a reason for hiding this comment

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

I moved some stuff around mostly into security, let me know if you like this layout better

docs/pipelines/multi-account/index.md Outdated Show resolved Hide resolved
docs/pipelines/multi-account/index.md Outdated Show resolved Hide resolved
josh-padnick
josh-padnick previously approved these changes Sep 28, 2023
Copy link
Contributor

@josh-padnick josh-padnick left a comment

Choose a reason for hiding this comment

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

These changes look great! The product is starting to shape up nicely. :)

As an aside, it may be worth explicitly linking to some of the security and multiple infra-live repos pages at the bottom of http://localhost:3000/pipelines/hello-world/#next-steps.

I left a few final comments. Please incorporate this feedback according to your judgment, and then let's merge and mark this done!

_docs-sources/pipelines/hello-world/github-enterprise.md Outdated Show resolved Hide resolved
_docs-sources/pipelines/hello-world/github-enterprise.md Outdated Show resolved Hide resolved

We recommend using a single `infrastructure-live` repository for managing your organization's infrastructure.
Sometimes, this isn't possible due to team structure, security requirements, or other limitations.
You may choose to use multiple repos to:
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
You may choose to use multiple repos to:
You may choose to use multiple `infrastructure-live` repos to:

_docs-sources/pipelines/security/multi-account.md Outdated Show resolved Hide resolved
@@ -0,0 +1,54 @@
# Multiple Infrastructure-Live Repos
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I personally find it easier to review and maintain the content when it's all on one line versus us manually adding line breaks.

Copy link
Author

Choose a reason for hiding this comment

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

sorry, bad habit from Garmin where they very strictly enforced the 80 character line limit even in text files

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh interesting. Yeah, let's standardize on one line then.

_docs-sources/pipelines/security/multi-account.md Outdated Show resolved Hide resolved
_docs-sources/pipelines/security/multi-account.md Outdated Show resolved Hide resolved
_docs-sources/pipelines/security/multi-account.md Outdated Show resolved Hide resolved
_docs-sources/pipelines/security/using-pipelines.md Outdated Show resolved Hide resolved
_docs-sources/pipelines/security/using-pipelines.md Outdated Show resolved Hide resolved
…ulti-repo

# Conflicts:
#	_docs-sources/pipelines/security/branch-protection.md
#	docs/pipelines/security/branch-protection.md
@oredavids
Copy link
Contributor

oredavids commented Sep 29, 2023

I would suggest re-running the regenerate docs command locally(yarn regenerate:local) if some suggestions were merged directly in GitHub. It does not seem that the docs-sourcer bot re-ran to update the docs folder.

@ellisonc ellisonc merged commit 90671c4 into master Sep 29, 2023
7 checks passed
@ellisonc ellisonc deleted the feature/CORE-1274-multi-repo branch September 29, 2023 17:50
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.

4 participants