-
Notifications
You must be signed in to change notification settings - Fork 13
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
Don't show ingress prompts in draft mode and add additional activity children to core draft commands #814
Conversation
…de-azurecontainerapps into mwf/homely-emerald
…ode-azurecontainerapps into mwf/fiscal-beige
…ode-azurecontainerapps into mwf/fiscal-beige
d4e2b15
to
eff6574
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logically it makes sense to me. My comments are suggestions, but I'm not going to try to require it.
}; | ||
wizardContext.telemetry.properties.revisionMode = containerApp.revisionsMode; | ||
|
||
if (isAzdExtensionInstalled()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this being added here? Doesn't seem to be related to the PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah not crucial to this PR at all, but it was small enough that I snuck it in. It was in the same snippet as the output logs code section
); | ||
|
||
// Log resource group | ||
wizardContext.activityChildren?.push( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like it might be time to make a utility function to just generate all of this boilerplate for you.
If not a util function, maybe include adding these activity children when you call pickContainerApp
or pickManagedEnvironment
? Since that seems to be where you are setting all of the wizard's contexts.
I feel like just adding them into the prompt steps themselves would be the best way to do it, but since these are top-level Azure resources, that might not be possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh interesting, I hadn't thought about doing it that way, but that might be a clean way to handle this boilerplate. I'll investigate that as a potential solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I consolidated most of the boilerplate under a new starting resource log step, let me know if you like that implementation or think it should be encapsulated differently!
519424d
to
eed858e
Compare
eed858e
to
06e1720
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely less concerned about StartingResourcesLogStep since it's always set to false for shouldPrompt 👍
Partially addresses part 2 of #719
Examples:
Edit container (edit w/ container registry image):
Edit container image (edit w/ workspace project image):