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.

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(host-collector): add progress for host collector #1659

Merged
merged 3 commits into from
Oct 25, 2024

Conversation

DexterYan
Copy link
Member

@DexterYan DexterYan commented Oct 23, 2024

Description, Motivation and Context

kots demo
Library | Loom - 24 October 2024 - Watch Video

sc-114419

Checklist

  • New and existing tests pass locally with introduced changes.
  • Tests for the changes have been added (for bug fixes / features)
  • The commit message(s) are informative and highlight any breaking changes
  • Any documentation required has been added/updated. For changes to https://troubleshoot.sh/ create a PR here

Does this PR introduce a breaking change?

  • Yes
  • No

@DexterYan DexterYan added the type::feature New feature or request label Oct 23, 2024
@DexterYan DexterYan requested a review from a team as a code owner October 23, 2024 05:07
@DexterYan DexterYan marked this pull request as draft October 23, 2024 05:07
@DexterYan DexterYan marked this pull request as ready for review October 24, 2024 02:19
Copy link
Member

@nvanthao nvanthao left a comment

Choose a reason for hiding this comment

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

Just add my thoughts,

I see we are using opts.CollectorProgressCallback because KOTS uses that callback to touch-up the the progress update.
So this change will have minimal impact to KOTS.

A right way I think, is not to use the callback, but to send into opts.ProgressChan a public struct collect.CollectProgress, and KOTS, as a consumer, will just use it. This will decouple KOTS and troubleshoot, no callback needed.

I see that we are already doing it for preflight

https://github.com/replicatedhq/kots/blob/99b056171e307ae50831d9ac9ddc71b91c84c12f/pkg/preflight/execute.go#L39-L53

That being said, KOTS will have to refactor with more code changes to cater for using collect.CollectProgress

Copy link
Member

@nvanthao nvanthao left a comment

Choose a reason for hiding this comment

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

Let's go with it now and we will have a separate card to refactor the progress logic in KOTS

@DexterYan DexterYan merged commit 350418c into main Oct 25, 2024
27 checks passed
@DexterYan DexterYan deleted the dx/sc-114419/update-process branch October 25, 2024 02:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type::feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants