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

Print flow installation commands #860

Merged
merged 8 commits into from
Nov 11, 2022
Merged

Print flow installation commands #860

merged 8 commits into from
Nov 11, 2022

Conversation

feluelle
Copy link
Member

@feluelle feluelle commented Nov 7, 2022 •
edited

Description

Currently, flow users are not seeing any progress made during the very first installation. This PR is addressing this by showing the commands for installing it - but only when the layers are not cached yet.

🎟 Issue(s)

Related astronomer/astro-sdk#1143

📸 Screenshots

First time running astro flow command:

Screenshot 2022-11-07 at 16 23 31

...

Screenshot 2022-11-07 at 16 25 39

When installed, it won't show the output as all of it is cached:

Screenshot 2022-11-07 at 16 22 07

📋 Checklist

  • Rebased from the main (or release if patching) branch (before testing)
  • Ran make test before taking out of draft
  • Ran make lint before taking out of draft
  • Added/updated applicable tests
  • Tested against Astro-API (if necessary).
  • Tested against Houston-API and Astronomer (if necessary).
  • Communicated to/tagged owners of respective clients potentially impacted by these changes.
  • Updated any related documentation

@feluelle
Print flow installation commands
55cb01d
@feluelle
Fix linting issue
8fb5dd6
@feluelle
Add tests
766df11 
- mock PrintBuildingSteps in basic tests
- add unittest for printBuildingSteps
@codecov
Copy link

codecov bot commented Nov 8, 2022 •
edited

Codecov Report

Base: 87.12% // Head: 87.11% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (edeb3c9) compared to base (a9097d3).
Patch coverage: 93.10% of modified lines in pull request are covered.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #860      +/-   ##
==========================================
- Coverage   87.12%   87.11%   -0.01%     
==========================================
  Files         103      103              
  Lines        8806     8827      +21     
==========================================
+ Hits         7672     7690      +18     
- Misses        661      663       +2     
- Partials      473      474       +1
Impacted Files Coverage Δ
sql/flow.go 91.76% <93.10%> (-1.99%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@feluelle
Refactor code
0237ddd 
- remove unused ioCopy for copying the build response
- always reset the function overwrites in tests to avoid side effects
@feluelle
Patch IoCopy in tests
b0a2620 
- remove unnecessary string array creation in tests
@feluelle feluelle marked this pull request as ready for review November 9, 2022 07:22
pankajkoti
pankajkoti reviewed Nov 9, 2022
View reviewed changes
Copy link
Contributor

@pankajkoti pankajkoti left a comment

Choose a reason for hiding this comment

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

Looks great to me, a couple of minor inline comments.

sql/flow_test.go Outdated Show resolved Hide resolved
sql/flow.go Outdated Show resolved Hide resolved
@feluelle
Copy link
Member Author

feluelle commented Nov 9, 2022

@pankajkoti I have addressed your comments. PTAL

neel-astro
neel-astro reviewed Nov 9, 2022
View reviewed changes
Copy link
Contributor

@neel-astro neel-astro left a comment

Choose a reason for hiding this comment

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

looks fine, have left minor nitpicks

sql/flow.go Outdated Show resolved Hide resolved
sql/flow.go Outdated Show resolved Hide resolved
sql/flow_test.go Outdated Show resolved Hide resolved
sql/flow.go Outdated Show resolved Hide resolved
@feluelle
Copy link
Member Author

feluelle commented Nov 9, 2022

Hey @neel-astro, thanks for your review. I have addressed your feedback. PTAL

neel-astro
neel-astro approved these changes Nov 10, 2022
View reviewed changes
Copy link
Contributor

@neel-astro neel-astro left a comment

Choose a reason for hiding this comment

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

LGTM

@tatiana
Copy link
Contributor

tatiana commented Nov 11, 2022

@feluelle, thanks for getting this done. It certainly improves the user experience.
I also liked the refactorings and tidying ups - but it may be better to those as part of a separate PR in future, since we end up squashing them :D

@neel-astro neel-astro merged commit 5234933 into main Nov 11, 2022
@neel-astro neel-astro deleted the feature/progress-visibility branch November 11, 2022 15:38
This was referenced Nov 16, 2022
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pankajkoti pankajkoti pankajkoti approved these changes

@neel-astro neel-astro neel-astro approved these changes

@andriisoldatenko andriisoldatenko Awaiting requested review from andriisoldatenko

@aliotta aliotta Awaiting requested review from aliotta

@AmFlint AmFlint Awaiting requested review from AmFlint

@kushalmalani kushalmalani Awaiting requested review from kushalmalani kushalmalani is a code owner

@sunkickr sunkickr Awaiting requested review from sunkickr sunkickr is a code owner

@jemishp jemishp Awaiting requested review from jemishp

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@feluelle @tatiana @pankajkoti @neel-astro