-
Notifications
You must be signed in to change notification settings - Fork 368
Stack management #4712
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
base: main
Are you sure you want to change the base?
Stack management #4712
Conversation
46b309e to
1a3a7c6
Compare
5fcdfda to
e2fc6df
Compare
6d8a01d to
f32f4bc
Compare
Signed-off-by: Rashed Kamal <rashed.kamal@broadcom.com>
Signed-off-by: Rashed Kamal <rashed.kamal@broadcom.com>
**V2 API endpoints must not expose or accept state parameter.** Signed-off-by: Rashed Kamal <rashed.kamal@broadcom.com>
Signed-off-by: Rashed Kamal <rashed.kamal@broadcom.com>
Signed-off-by: Rashed Kamal <rashed.kamal@broadcom.com>
Signed-off-by: Rashed Kamal <rashed.kamal@broadcom.com>
and integration into app creation and build workflows. Signed-off-by: Rashed Kamal <rashed.kamal@broadcom.com>
Signed-off-by: Rashed Kamal <rashed.kamal@broadcom.com>
Signed-off-by: Rashed Kamal <rashed.kamal@broadcom.com>
Signed-off-by: Rashed Kamal <rashed.kamal@broadcom.com>
aa5f9b9 to
a1eab88
Compare
…APIs Sets warnings in the response hash when there are warnings Signed-off-by: Rashed Kamal <rashed.kamal@broadcom.com>
a1eab88 to
3b29b53
Compare
|
Thanks for the PR - i adapted the RFC i also think the new state naming fits better 👍🏼 cloudfoundry/community#1405 |
| "updated_at": "2018-11-09T22:43:28Z", | ||
| "name": "my-stack", | ||
| "description": "Here is my stack!", | ||
| "state": "ACTIVE", |
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.
suggestion It would be good to have a definition of each state and what it means for the user. Perhaps in https://v3-apidocs.cloudfoundry.org/version/3.209.0/index.html#stacks description?
| module VCAP::CloudController | ||
| class StackUpdateMessage < MetadataBaseMessage | ||
| register_allowed_keys [] | ||
| register_allowed_keys [:state] |
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.
question In the RFC it says
To improve communication with developers, the existing description field of a stack will be leveraged. The content of the description field should be included in any warning or error message related to the stack's state. This allows operators to provide context, migration guides, or links to further documentation.
Should we allow operators to update the description to set a user friendly message?
|
|
||
| ```shell | ||
| curl "https://api.example.org/v3/stacks/[guid]" \ | ||
| -X PATCH \ |
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.
issue Are you missing docs for create stack?
| Name | Type | Description | ||
| ---- | ---- | ----------- | ||
| **metadata.labels** | [_label object_](#labels) | Labels applied to the stack | ||
| **metadata.annotations** | [_annotation object_](#annotations) | Annotations applied to the stack |
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.
issue Are you missing docs for state here?
| @@ -0,0 +1,32 @@ | |||
| module VCAP::CloudController | |||
| class StackStateValidator | |||
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.
question Similar to above, as per
To improve communication with developers, the existing description field of a stack will be leveraged. The content of the description field should be included in any warning or error message related to the stack's state. This allows operators to provide context, migration guides, or links to further documentation.
Do we also want to include the stack description in the warning?
| @@ -0,0 +1,17 @@ | |||
| module VCAP::CloudController | |||
| class StackStates | |||
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.
question Any reason for this to be a separate class? It seems to me to make sense to be in the main model file. See BuildModel and DeploymentModel as examples.
| DEFAULT_CONTAINER_USER = 'vcap'.freeze | ||
| DEFAULT_DOCKER_CONTAINER_USER = 'root'.freeze | ||
|
|
||
| attr_reader :stack_warnings |
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.
thought Not sure how I feel about storing the warnings on all the models here. It feels like its leaking an abstraction, especially since the warning generation is not actually happening in the model itself but as part of the action.
Perhaps this should be on the actions themselves? So the warnings are kept purely between the controller/action? Will think about it and see if theres any existing patterns for this.
A short explanation of the proposed change:
Implement stack lifecycle management with ACTIVE, DEPRECATED, RESTRICTED, and DISABLED states to enable gradual stack deprecation without causing application downtime.
An explanation of the use cases your change solves
Implement the Stack Management RFC by adding a
statefield to stacks with four lifecycle states: ACTIVE, DEPRECATED, RESTRICTED, and DISABLED. This enables operators to gracefully phase out stacks through lifecycle restrictions rather than causing immediate app downtimeLinks to any other associated PRs
This PR implements RFC #0045.
While the
RESTRICTEDstate is not explicitly defined in the RFC, it is functionally identical toLOCKED. We chose the termRESTRICTEDto prevent any confusion with the existing 'Locked' state used in Buildpacks.I have reviewed the contributing guide
I have viewed, signed, and submitted the Contributor License Agreement
I have made this pull request to the
mainbranchI have run all the unit tests using
bundle exec rakeI have run CF Acceptance Tests