Skip to content

Updates related to EnvironmentVariablesListStep #450

Merged
MicroFish91 merged 32 commits intomainfrom
mwf/deployWorkspaceProject-IV
Sep 15, 2023
Merged

Updates related to EnvironmentVariablesListStep #450
MicroFish91 merged 32 commits intomainfrom
mwf/deployWorkspaceProject-IV

Conversation

@MicroFish91
Copy link
Copy Markdown
Contributor

@MicroFish91 MicroFish91 commented Sep 15, 2023

Continuation of: #425

  • Consolidate two steps into one step:
    image

  • Add/use static method for checking if workspace has an env file

@MicroFish91 MicroFish91 changed the base branch from main to mwf/deployWorkspaceProject-III September 15, 2023 16:52
@MicroFish91 MicroFish91 force-pushed the mwf/deployWorkspaceProject-IV branch from 2d7d0fa to 73204e9 Compare September 15, 2023 16:57
@MicroFish91 MicroFish91 force-pushed the mwf/deployWorkspaceProject-IV branch from f76cb4d to 541af13 Compare September 15, 2023 17:18
@MicroFish91 MicroFish91 marked this pull request as ready for review September 15, 2023 17:27
@MicroFish91 MicroFish91 requested a review from a team as a code owner September 15, 2023 17:27
Copy link
Copy Markdown
Member

@nturinski nturinski left a comment

Choose a reason for hiding this comment

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

Somewhat related to this PR-- do you know if you hit "skip for now", does the container app keep its existing environment settings or does that get reset?

@MicroFish91
Copy link
Copy Markdown
Contributor Author

MicroFish91 commented Sep 15, 2023

I think currently it would just have no environment variables if you hit "skip for now". Think it would be worth adding a logic path to carry over the settings without the .env?

@nturinski
Copy link
Copy Markdown
Member

Yeah, this came up in a service connector meeting. Deploying to a container app and resetting all the environment settings is a pretty bad experience because it'd break a lot of things that were previously working. I don't know if we should have another option that says "use existing settings" or, if you just skip it, it preserves whatever settings were on the app.

// Todo: It might be nice to add a direct command to update just the environment variables rather than having to suggest to re-run the entire command again
private outputLogs(_context: ImageSourceBaseContext, setEnvironmentVariableOption: SetEnvironmentVariableOption): void {
if (setEnvironmentVariableOption !== SetEnvironmentVariableOption.ProvideFile) {
// context.activityChildren?.push(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why did you comment out of the activity log stuff? Not hooked up yet?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yup, going to hook up all the activity children logic in one PR later down the line because it's a bit more involved than just these item pushes... figured it's easier to review together in one wombo combo

@MicroFish91
Copy link
Copy Markdown
Contributor Author

Yeah, this came up in a service connector meeting. Deploying to a container app and resetting all the environment settings is a pretty bad experience because it'd break a lot of things that were previously working. I don't know if we should have another option that says "use existing settings" or, if you just skip it, it preserves whatever settings were on the app.

That makes sense, I'll open an issue

Base automatically changed from mwf/deployWorkspaceProject-III to main September 15, 2023 19:04
@MicroFish91 MicroFish91 merged commit a29191c into main Sep 15, 2023
@MicroFish91 MicroFish91 deleted the mwf/deployWorkspaceProject-IV branch September 15, 2023 19:12
@microsoft microsoft locked and limited conversation to collaborators Mar 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants