-
Notifications
You must be signed in to change notification settings - Fork 900
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(core/managed): add Managed Resources section to app config, allow opting out #7409
Conversation
The following commits need their title changed:
Please format your commit title into the form:
This allows us to easily generate changelogs & determine semantic version numbers when cutting releases. You can read more about commit conventions here. |
Thanks for tagging me! Looks great and I wouldn't worry about the async loading. I would consider changing: "Spinnaker is configured to continuously manage some resources" to "Spinnaker is configured to continuously manage some of this application's resources" |
Love the rainbow. As I look at the "paused" vs "active" screens, I think the fact that the buttons looks so similar is a little confusing. I guess in addition to the rainbow I'd love a stop sign or something in front of "continuous management of resources is paused" to make it easier to quickly scan and tell the state of managment |
@emjburns @gcomstock another rev with some wording + visual tweaks: |
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.
nice
if (this.feature.managedResources) { | ||
this.hasManagedResources = false; | ||
ManagedReader.getApplicationSummary(this.application.name).then(({ hasManagedResources }) => { | ||
$scope.$applyAsync(() => { |
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 surprised to see applyAsync all over this file, because the file is written in angular. I thought applyAsync was generally used when react code wants to make AngularJS code aware of data changes. Do you have any idea what's going on here?
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.
recording an in-person chat for posterity: we have no idea why. it's probably because of Some Subtle State Bug(tm) that popped up and we've been copy-pasting the same logic ever since.
{(!pausePending && <i className={classNames('fa sp-margin-xs-right', iconClass)} />) || <ButtonBusyIndicator />}{' '} | ||
{paused ? 'Resume Management' : 'Pause Management'} | ||
</button> | ||
{pauseFailed && <div className="alert alert-danger sp-margin-l-top">Saving failed.</div>} |
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.
can this use the <ValidationMessage />
component? (which I totally want to rename to simply <Message/>
)
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.
68cc18f fix(core): Sanitize confirmation modal body (spinnaker#7407) f8267a4 feat(core/managed): add Managed Resources section to app config, allow opting out (spinnaker#7409)
68cc18f fix(core): Sanitize confirmation modal body (spinnaker#7407) f8267a4 feat(core/managed): add Managed Resources section to app config, allow opting out (spinnaker#7409)
depends on spinnaker/gate#898
This adds a new, feature flagged "Managed Resources" section to the app config, which initially supports one thing — 'pausing' management of resources through our new
veto
concept. The section only shows up if two things are true:feature.managedResources
turned onThis is slightly awkward because it means that if you do have managed resources, that app config section 'pops' in async. In my experience it's not an issue and the plan is to do a more static configuration (where you always see the section if the feature flag is on) once this is less experimental.
active:
![active](https://user-images.githubusercontent.com/1850998/64992828-8f6e4b00-d889-11e9-88aa-c233477b8e09.png)
paused:
![paused](https://user-images.githubusercontent.com/1850998/64992846-985f1c80-d889-11e9-892e-028a04188d07.png)
cc @emjburns @robfletcher @asher @gcomstock