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.

Sign up for GitHub

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

Jump to bottom

[azure-docs] Deploying with ACR. #26187

Merged
merged 3 commits into from
Dec 5, 2024
Merged

[azure-docs] Deploying with ACR. #26187

merged 3 commits into from
Dec 5, 2024

Conversation

dpeng817
Copy link
Contributor

@dpeng817 dpeng817 commented Nov 27, 2024 •
edited
Loading

Summary & Motivation

A guide on deploying dagster code locations using azure container registry.

Similarly to the previous PR, mostly focuses on the necessary azure incantations. Want someone to vet that nothing here is bad practice.

How I tested this

I deployed code locations myself using this method.

@dpeng817 Graphite App
Copy link
Contributor Author

dpeng817 commented Nov 27, 2024 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

@dpeng817 dpeng817 mentioned this pull request Nov 27, 2024
Merged
This was referenced Nov 27, 2024
Merged
Merged
Merged
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 27, 2024 •
edited
Loading

Deploy preview for dagster-docs ready!

Preview available at https://dagster-docs-i59t1ud76-elementl.vercel.app
https://dpeng817-user-code.dagster.dagster-docs.io

Direct link to changed pages:

@dpeng817 dpeng817 mentioned this pull request Nov 28, 2024
Merged
@dpeng817 dpeng817 marked this pull request as ready for review December 3, 2024 18:54
@dpeng817 dpeng817 requested a review from neverett as a code owner December 3, 2024 18:54
@graphite-app graphite-app bot added the docs-to-migrate Docs to migrate to new docs site label Dec 3, 2024
@graphite-app Graphite App
Copy link

graphite-app bot commented Dec 3, 2024

Graphite Automations

"Add a 'docs-to-migrate' label to PRs with docs" took an action on this PR • (12/03/24)

1 label was added to this PR based on Christopher DeCarolis's automation.

@dpeng817 dpeng817 mentioned this pull request Dec 3, 2024
Closed
@dpeng817 dpeng817 requested a review from yuhan December 3, 2024 22:54
mlarose
mlarose previously requested changes Dec 3, 2024
View reviewed changes
Copy link
Contributor

@mlarose mlarose left a comment •
edited
Loading

Choose a reason for hiding this comment

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

my point about other CI/CD needs to be addressed.

otherwise, I am wondering how much we want to duplicate and maintain the documentation that is already there: https://docs.dagster.io/dagster-plus/getting-started#step-4-configure-cicd-for-your-project vs merging them or cross referencing through them.

However, there is merit to having this in a step by step guide, as the user doesn't have to navigate so much as in the existing documentation. But it might get tedious to keep up-to-date in the future. I guess that's a question for @neverett to chime in.

docs/content/dagster-plus/deployment/azure/acr-user-code.mdx Outdated Show resolved Hide resolved
@dpeng817 Graphite App
Copy link
Contributor Author

dpeng817 commented Dec 4, 2024

Yea I think that eventually we want to figure out how to merge everything together in a more coherent way.

But I think the way that most users will interact with the docs site is either we'll send them a particular page, or they'll search something up. And I want to make sure there's a linear pathway when they search up azure, or alternatively sales can just send this single guide to a user to get them onboarded. I think that the consolidation piece can come later.

@slopp
Copy link
Contributor

slopp commented Dec 4, 2024

I strongly agree with this.

Our current docs have you constantly chasing links.

I believe Nikki calls it "stand alone pages". Each page doesn't need to contain the whole world, but it should be useful on its own.

@dpeng817 dpeng817 force-pushed the dpeng817/user_code branch 2 times, most recently from ce3da57 to 515e1da Compare December 4, 2024 22:06
@dpeng817
[azure-docs] Deploying with ACR.
f1a6d00
@dpeng817
[azure-docs] compute log manager docs (#26202)
36e0b85 
## Summary & Motivation
Add docs on using the azure blob storage compute log manager in AKS.

## How I Tested These Changes
I followed these instructions myself to get it set up.
@dpeng817
[azure-docs] compute log manager docs
477cf53
Base automatically changed from dpeng817/aks_agent to dpeng817/fix_compute_log_manager December 5, 2024 19:43
@dpeng817 dpeng817 merged commit 477cf53 into dpeng817/fix_compute_log_manager Dec 5, 2024
1 of 2 checks passed
@dpeng817 dpeng817 deleted the dpeng817/user_code branch December 5, 2024 19:43
graphite-app[bot]
graphite-app bot reviewed Dec 5, 2024
View reviewed changes
docs/content/dagster-plus/deployment/azure/blob-compute-logs.mdx
--identity-name agent-identity \
--resource-group <resource-group> \
--issuer $(az aks show -g <resource-group> -n <aks-cluster-name> --query "oidcIssuerProfile.issuerUrl" -otsv) \
--subject system:serviceaccount:dagster-agent:dagster-agent-service-account
Copy link

Choose a reason for hiding this comment

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

The --subject parameter references dagster-agent-service-account but this service account was created earlier with the name dagster-agent-sa. To maintain consistency, the command should be updated to:

--subject system:serviceaccount:dagster-agent:dagster-agent-sa

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

@neverett neverett removed the docs-to-migrate Docs to migrate to new docs site label Jan 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@graphite-app graphite-app[bot] graphite-app[bot] left review comments

@mlarose mlarose mlarose approved these changes

@gibsondan gibsondan Awaiting requested review from gibsondan

@slopp slopp Awaiting requested review from slopp

@neverett neverett Awaiting requested review from neverett

@yuhan yuhan Awaiting requested review from yuhan

@cnolanminich cnolanminich Awaiting requested review from cnolanminich

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dpeng817 @slopp @mlarose @cnolanminich @neverett