Skip to content

Prevent deployment status string accumulation#5707

Closed
ironcladlou wants to merge 1 commit intoopenshift:masterfrom
ironcladlou:details-append-fix
Closed

Prevent deployment status string accumulation#5707
ironcladlou wants to merge 1 commit intoopenshift:masterfrom
ironcladlou:details-append-fix

Conversation

@ironcladlou
Copy link
Contributor

Rebuild the deployment config status from scratch each time to prevent
accumulating the string with old messages. Separate the algorithm into
two parts: one which builds up the context for the details, and one which
renders the context into a string.

Fixes #5706

@ironcladlou
Copy link
Contributor Author

Unit test forthcoming

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

your capitalization is wrong on your errors

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already fixed with my unit test commit

Rebuild the deployment config status from scratch each time to prevent
accumulating the string with old messages. Separate the algorithm into
two parts: one which builds up the context for the details, and one which
renders the context into a string.
@smarterclayton smarterclayton added priority/P1 component/apps area/reliability approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 4, 2015
@smarterclayton
Copy link
Contributor

Actually, hold on the merge - will this fetch all build configs every time there is an error now?

@smarterclayton
Copy link
Contributor

In a loop?

@ironcladlou
Copy link
Contributor Author

It USED to load build configs for every trigger every time it handled the config. Now it does it just once per handle, which is still too much. Hence the switch to a store in #5530

@smarterclayton
Copy link
Contributor

Introduced by #4941

@smarterclayton smarterclayton removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 4, 2015
@ironcladlou
Copy link
Contributor Author

This will be rolled into #5530 and is superceded by #5713 for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants