Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(helptext): clarify text for stage timeout #7336

Merged
merged 3 commits into from
Oct 30, 2019
Merged

fix(helptext): clarify text for stage timeout #7336

merged 3 commits into from
Oct 30, 2019

Conversation

marchello2000
Copy link
Contributor

No description provided.

@ajordens
Copy link
Contributor

Not sure if we want to have the examples listed as bullet points ... but I appreciate the wording change to remove "task" and introduce "sensible".

That strikes me as a happy medium without investing in better (server-side) calculation.

@marchello2000
Copy link
Contributor Author

tweaked to this:
image

@erikmunson
Copy link
Member

After discussing this at length when we originally made the wording change, we moved away from stage-specific examples like these because it causes similar problems to implying there's a predictable default timeout for stages. AFAICT talking about how long stages typically take is either 1) vague and doesn't build confidence/predictability or 2) too nuanced and specific and causes more confusion (e.g. I am pretty confident the 2-hour deploy stage example would've still led to the internal issues we saw with RRB-based deploys where folks didn't expect a multi-hour timeout due to pipeline runs).

I'd advocate for continuing to avoid specific examples but tweaking the wording about task timeouts to be closer to this proposal re: sensible defaults, as I 100% agree that part is a clearer articulation of the real behavior. For example: Note: By default, Spinnaker will use sensible timeouts that depend on the stage type and the operations the stage needs to perform at runtime. These defaults can vary based on chosen configuration and other external factors.

@marchello2000
Copy link
Contributor Author

@erikmunson : i (after months of dilly-dallying) incorporated your suggestion.

@marchello2000 marchello2000 added the ready to merge Reviewed and ready for merge label Oct 30, 2019
@erikmunson
Copy link
Member

Love it, thanks Mark!

@mergify mergify bot added the auto merged Merged automatically by a bot label Oct 30, 2019
@mergify mergify bot merged commit 8d478a0 into spinnaker:master Oct 30, 2019
@marchello2000 marchello2000 deleted the mark/timeouttext branch October 30, 2019 20:46
erikmunson pushed a commit to erikmunson/deck that referenced this pull request Nov 6, 2019
chore(core): Bump version to 0.0.429
0cf5f75 
5acb0d0 feat(managed): Add status popovers, new props for HoverablePopover
88b8e6c feat(managed): add resource status indicator to security groups
f02b70d feat(managed): add managed resource data to security group groups
14283b2 feat(managed): add resource status indicator to load balancers
aaa349b feat(managed): add managed resource data to load balancer groups
a6d7756 feat(managed): add resource status indicator to clusters
8453260 feat(managed): add managed resource data to cluster groups
167a151 feat(dataSources): add runtime error when defaultData isn't provided (spinnaker#7591)
a10e068 feat(core/presentation): Reduce unnecessary renders in useLatestPromise, add tests
90b249d feat(core/presentation): Reduce unnecessary renders in useDebouncedValue, add tests
7ff7aa1 refactor(core/presentation): extract useIsMountedRef hook
b18b577 chore(package): update to @types/enzyme@3.10.3, enzyme@3.10.0, enzyme-adapter-react-16@1.15.1 (spinnaker#7585)
df7120a chore(help): Update help.contents.ts (spinnaker#7588)
98e8a4b fix(rerun): Hiding re-run as strategies should not be re-run (spinnaker#7583)
8d478a0 fix(helptext): clarify text for stage timeout (spinnaker#7336)
@erikmunson erikmunson mentioned this pull request Nov 6, 2019
Merged
erikmunson pushed a commit that referenced this pull request Nov 6, 2019
chore(core): Bump version to 0.0.429 (#7596)
1108749 
5acb0d0 feat(managed): Add status popovers, new props for HoverablePopover
88b8e6c feat(managed): add resource status indicator to security groups
f02b70d feat(managed): add managed resource data to security group groups
14283b2 feat(managed): add resource status indicator to load balancers
aaa349b feat(managed): add managed resource data to load balancer groups
a6d7756 feat(managed): add resource status indicator to clusters
8453260 feat(managed): add managed resource data to cluster groups
167a151 feat(dataSources): add runtime error when defaultData isn't provided (#7591)
a10e068 feat(core/presentation): Reduce unnecessary renders in useLatestPromise, add tests
90b249d feat(core/presentation): Reduce unnecessary renders in useDebouncedValue, add tests
7ff7aa1 refactor(core/presentation): extract useIsMountedRef hook
b18b577 chore(package): update to @types/enzyme@3.10.3, enzyme@3.10.0, enzyme-adapter-react-16@1.15.1 (#7585)
df7120a chore(help): Update help.contents.ts (#7588)
98e8a4b fix(rerun): Hiding re-run as strategies should not be re-run (#7583)
8d478a0 fix(helptext): clarify text for stage timeout (#7336)
Jammy-Louie pushed a commit to pivotal/deck that referenced this pull request Nov 8, 2019
@marchello2000
fix(helptext): clarify text for stage timeout (spinnaker#7336)
9110beb
Jammy-Louie pushed a commit to pivotal/deck that referenced this pull request Nov 8, 2019
chore(core): Bump version to 0.0.429 (spinnaker#7596)
9aa2039 
5acb0d0 feat(managed): Add status popovers, new props for HoverablePopover
88b8e6c feat(managed): add resource status indicator to security groups
f02b70d feat(managed): add managed resource data to security group groups
14283b2 feat(managed): add resource status indicator to load balancers
aaa349b feat(managed): add managed resource data to load balancer groups
a6d7756 feat(managed): add resource status indicator to clusters
8453260 feat(managed): add managed resource data to cluster groups
167a151 feat(dataSources): add runtime error when defaultData isn't provided (spinnaker#7591)
a10e068 feat(core/presentation): Reduce unnecessary renders in useLatestPromise, add tests
90b249d feat(core/presentation): Reduce unnecessary renders in useDebouncedValue, add tests
7ff7aa1 refactor(core/presentation): extract useIsMountedRef hook
b18b577 chore(package): update to @types/enzyme@3.10.3, enzyme@3.10.0, enzyme-adapter-react-16@1.15.1 (spinnaker#7585)
df7120a chore(help): Update help.contents.ts (spinnaker#7588)
98e8a4b fix(rerun): Hiding re-run as strategies should not be re-run (spinnaker#7583)
8d478a0 fix(helptext): clarify text for stage timeout (spinnaker#7336)
sergiopena pushed a commit to sergiopena/deck that referenced this pull request Jan 10, 2020
@marchello2000 @sergiopena
fix(helptext): clarify text for stage timeout (spinnaker#7336)
e5a2228
sergiopena pushed a commit to sergiopena/deck that referenced this pull request Jan 10, 2020
@sergiopena
chore(core): Bump version to 0.0.429 (spinnaker#7596)
0b300a6 
5acb0d0 feat(managed): Add status popovers, new props for HoverablePopover
88b8e6c feat(managed): add resource status indicator to security groups
f02b70d feat(managed): add managed resource data to security group groups
14283b2 feat(managed): add resource status indicator to load balancers
aaa349b feat(managed): add managed resource data to load balancer groups
a6d7756 feat(managed): add resource status indicator to clusters
8453260 feat(managed): add managed resource data to cluster groups
167a151 feat(dataSources): add runtime error when defaultData isn't provided (spinnaker#7591)
a10e068 feat(core/presentation): Reduce unnecessary renders in useLatestPromise, add tests
90b249d feat(core/presentation): Reduce unnecessary renders in useDebouncedValue, add tests
7ff7aa1 refactor(core/presentation): extract useIsMountedRef hook
b18b577 chore(package): update to @types/enzyme@3.10.3, enzyme@3.10.0, enzyme-adapter-react-16@1.15.1 (spinnaker#7585)
df7120a chore(help): Update help.contents.ts (spinnaker#7588)
98e8a4b fix(rerun): Hiding re-run as strategies should not be re-run (spinnaker#7583)
8d478a0 fix(helptext): clarify text for stage timeout (spinnaker#7336)
yunzhangit pushed a commit to yunzhangit/deck that referenced this pull request Mar 28, 2021
@marchello2000 @yunzhangit
fix(helptext): clarify text for stage timeout (spinnaker#7336)
e863ff5
yunzhangit pushed a commit to yunzhangit/deck that referenced this pull request Mar 28, 2021
@yunzhangit
chore(core): Bump version to 0.0.429 (spinnaker#7596)
fa861f5 
5acb0d0 feat(managed): Add status popovers, new props for HoverablePopover
88b8e6c feat(managed): add resource status indicator to security groups
f02b70d feat(managed): add managed resource data to security group groups
14283b2 feat(managed): add resource status indicator to load balancers
aaa349b feat(managed): add managed resource data to load balancer groups
a6d7756 feat(managed): add resource status indicator to clusters
8453260 feat(managed): add managed resource data to cluster groups
167a151 feat(dataSources): add runtime error when defaultData isn't provided (spinnaker#7591)
a10e068 feat(core/presentation): Reduce unnecessary renders in useLatestPromise, add tests
90b249d feat(core/presentation): Reduce unnecessary renders in useDebouncedValue, add tests
7ff7aa1 refactor(core/presentation): extract useIsMountedRef hook
b18b577 chore(package): update to @types/enzyme@3.10.3, enzyme@3.10.0, enzyme-adapter-react-16@1.15.1 (spinnaker#7585)
df7120a chore(help): Update help.contents.ts (spinnaker#7588)
98e8a4b fix(rerun): Hiding re-run as strategies should not be re-run (spinnaker#7583)
8d478a0 fix(helptext): clarify text for stage timeout (spinnaker#7336)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erikmunson erikmunson erikmunson approved these changes

@ajordens ajordens Awaiting requested review from ajordens

Assignees
No one assigned
Labels
auto merged Merged automatically by a bot ready to merge Reviewed and ready for merge target-release/1.18
Projects
None yet
Milestone
No milestone
4 participants
@marchello2000 @ajordens @erikmunson @spinnakerbot