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
fix(9656): stores all states except workflows, fixes #9656 #9846
fix(9656): stores all states except workflows, fixes #9656 #9846
Conversation
2c5b1f1
to
f1dd563
Compare
Signed-off-by: Athitya Kumar <athityakumar@gmail.com>
f1dd563
to
139d75f
Compare
ui/src/app/archived-workflows/components/archived-workflow-list/archived-workflow-list.tsx
Outdated
Show resolved
Hide resolved
ui/src/app/archived-workflows/components/archived-workflow-list/archived-workflow-list.tsx
Show resolved
Hide resolved
ui/src/app/archived-workflows/components/archived-workflow-list/archived-workflow-list.tsx
Outdated
Show resolved
Hide resolved
Signed-off-by: Athitya Kumar <athityakumar@gmail.com>
Signed-off-by: Athitya Kumar <athityakumar@gmail.com>
private saveHistory() { | ||
this.storage.setItem('options', this.state, {} as State); | ||
this.storage.setItem('options', this.fetchBrowserStorageStateObject(this.state), {} as 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.
I don't think this is the correct approach. A new objects should be constructed with only allowed fields.
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.
interface Options {
namespace: string
name: string
namePrefix: string
selectedPhases: string[]
selectedLabels: string[]
minStartedAt: Date
maxStartedAt: Date
pagination: Pagination
}
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.
@alexec - But that'd mean we have to set the attributes manually for the new options
variable, which is an Options
interface object right? @juliev0 had raised a concern earlier that manually specifying attributes may have a maintenance overhead (if/when we add a new attribute to State
interface, we'd also have to add it to the Options
interface and in the function that converts the State
object to Options
object) - due to which I tried to automate the logic by taking blacklistedStates
as an input.
Please lmk if Options
is the preferred approach - I can update my PR with the same changes 😄
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.
Ping @alexec @sarabala1979 ^
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 think it preferable to construct new object to write lots of code to black list fields. Less code to maintain I think.
@sarabala1979 @alexec - Any next steps on this PR? Please lmk if any action is required from my side 😄 |
Signed-off-by: Athitya Kumar <athityakumar@gmail.com>
Signed-off-by: Athitya Kumar <athityakumar@gmail.com>
Head branch was pushed to by a user without write access
Fixes #9656
Please do not open a pull request until you have checked ALL of these:
make pre-commit -B
to fix codegen and lint problems.The
workflows
key/value is no longer stored in browser's local storage for archived workflows page, after this change:If changes were requested, and you've made them, dismiss the review to get it reviewed again.