-
Notifications
You must be signed in to change notification settings - Fork 902
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
feat(provider/google): Added Cloud Run manifest functionality in Deck. #9971
Conversation
@mattgogerly could you please review the PR |
{statusKey} | ||
</span> | ||
)} | ||
{(isStable || isUnstableWithMessage) && <span> </span>} |
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'm not sure this does anything useful?
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.
Removed unwanted condition check
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.
This seems to duplicate the Kubernetes equivalent. Can we generify that instead of having the same code twice?
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.
Yes , for now we are using this component in 2 modules (i.e, Kubernetes and Cloud Run) and we can generify that as well But in future if Kubernetes modifies the component based on some new requirement will affect Cloud Run module as well or vice-versa. So we are trying to keep the scope of components to module-centric, So that modification in any one module will not affect other modules.
import type { IDeploymentStrategy } from '@spinnaker/core'; | ||
|
||
export const strategyRedBlack: IDeploymentStrategy = { | ||
label: 'Red/Black (Deprecated)', |
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 red/black deprecated?
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.
Corrected label text.
@@ -5,6 +5,7 @@ export interface IArtifactTypePatterns { | |||
export const ArtifactTypePatterns: IArtifactTypePatterns = { | |||
BITBUCKET_FILE: /bitbucket\/file/, | |||
CUSTOM_OBJECT: /custom\/object/, | |||
CLOUDRUN: /cloudrun\/(.*)/, |
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.
Forgive me if I'm not familiar with Cloud Run. Why does it have its own artifact type if the manifest is just YAML/JSON?
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.
So based on back-end implementation CLOUDRUN type is not much useful for now like KUBERNETES, So removing it for now will add it if required in future based on enhancement request .
spinnaker#9971) * feat(cloudRun): Implemented manifest stage for cloudRun * feat(cloudRun): Incorporated suggested changes
spinnaker#9971) * feat(cloudRun): Implemented manifest stage for cloudRun * feat(cloudRun): Incorporated suggested changes
This is part of spinnaker/governance#302
Added Cloud Run manifest functionality in Deck.