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

close progress with stdin #24280

Merged
merged 5 commits into from
Dec 5, 2024
Merged

close progress with stdin #24280

merged 5 commits into from
Dec 5, 2024

Conversation

mostlikelee
Copy link
Contributor

@mostlikelee mostlikelee commented Dec 2, 2024

#24212

The zenity progress window expects stdin to update and close the window, this caused a few issues where

  1. zenity was not recognizing some flags
  2. the zenity process was forced to be killed as it did not respect a context cancellation

creating a StdinPipe() and closing it fixes both issues

  • Changes file added for user-visible changes in changes/, orbit/changes/ or ee/fleetd-chrome/changes.
    See Changes files for more information.
  • Added/updated tests
  • Manual QA for all new/changed functionality
  • For Orbit and Fleet Desktop changes:
    • Orbit runs on macOS, Linux and Windows. Check if the orbit feature/bugfix should only apply to one platform (runtime.GOOS).
    • Manual QA must be performed in the three main OSs, macOS, Windows and Linux.
    • Auto-update manual QA, from released version of component to new version (see tools/tuf/test).

Copy link

codecov bot commented Dec 3, 2024

Codecov Report

Attention: Patch coverage is 10.71429% with 50 lines in your changes missing coverage. Please review.

Project coverage is 63.42%. Comparing base (b482223) to head (c39aea3).
Report is 45 commits behind head on main.

Files with missing lines Patch % Lines
orbit/pkg/execuser/execuser_linux.go 0.00% 22 Missing ⚠️
orbit/pkg/luks/luks_linux.go 0.00% 15 Missing ⚠️
orbit/pkg/zenity/zenity.go 46.15% 7 Missing ⚠️
orbit/pkg/execuser/execuser.go 0.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #24280      +/-   ##
==========================================
- Coverage   63.43%   63.42%   -0.01%     
==========================================
  Files        1584     1584              
  Lines      150498   150621     +123     
  Branches     3771     3824      +53     
==========================================
+ Hits        95465    95536      +71     
- Misses      47415    47460      +45     
- Partials     7618     7625       +7     
Flag Coverage Δ
backend 64.29% <10.71%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mostlikelee mostlikelee marked this pull request as ready for review December 3, 2024 23:33
Copy link
Member

@lucasmrod lucasmrod left a comment

Choose a reason for hiding this comment

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

LGTM! Just two small comments to address.

.github/workflows/fleet-and-orbit.yml Show resolved Hide resolved
Title: infoTitle,
Text: "Validating passphrase...",
})
if err != nil {
log.Error().Err(err).Msg("failed to show progress dialog")
}
defer func() {
if err := cancelProgress(); err != nil {
log.Error().Err(err).Msg("failed to cancel progress dialog")
Copy link
Member

Choose a reason for hiding this comment

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

This will error because the (stdin) file is already closed, so maybe change this one to log.Debug()? (to not create noise)

}
}

// ShowEntry displays an dialog that accepts end user input. It returns the entered
// text or errors ErrCanceled, ErrTimeout, or ErrUnknown.
func (z *Zenity) ShowEntry(ctx context.Context, opts dialog.EntryOptions) ([]byte, error) {
z.killZenityFunc()
Copy link
Member

Choose a reason for hiding this comment

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

Happy to see this go away :)

Copy link
Member

@lucasmrod lucasmrod left a comment

Choose a reason for hiding this comment

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

LGTM!

@mostlikelee
Copy link
Contributor Author

@sharon-fdm need a review from you as it's updating CI to use macos-13 (macos-12 is EOL)

Copy link
Collaborator

@sharon-fdm sharon-fdm left a comment

Choose a reason for hiding this comment

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

I only reviewed .github/workflows/fleet-and-orbit.yml
Approving based on Lucas' review

@mostlikelee mostlikelee merged commit 7547dcb into main Dec 5, 2024
36 checks passed
@mostlikelee mostlikelee deleted the 24212-fix-progress branch December 5, 2024 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants