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

Astro Run Command #776

Merged
merged 126 commits into from
Nov 21, 2022
Merged

Astro Run Command #776

merged 126 commits into from
Nov 21, 2022

Conversation

sunkickr
Copy link
Contributor

@sunkickr sunkickr commented Oct 3, 2022

Description

new astro run command

inlcudes dags and changes to the docker file that are for testing purposes only. These changes will be removed once the command is merged

🎟 Issue(s)

🧪 Functional Testing

Added tests for new functions

📸 Screenshots

Add screenshots to illustrate the validity of these changes.

📋 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

@codecov
Copy link

codecov bot commented Oct 11, 2022

Codecov Report

Base: 87.19% // Head: 87.20% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (331a6fc) compared to base (04bacf0).
Patch coverage: 84.34% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #776      +/-   ##
==========================================
+ Coverage   87.19%   87.20%   +0.01%     
==========================================
  Files         107      108       +1     
  Lines        9107     9294     +187     
==========================================
+ Hits         7941     8105     +164     
- Misses        688      700      +12     
- Partials      478      489      +11     
Impacted Files Coverage Δ
airflow/container.go 67.24% <ø> (ø)
cloud/deploy/deploy.go 73.52% <ø> (ø)
airflow/docker.go 85.03% <71.42%> (-1.44%) ⬇️
settings/settings.go 83.80% <80.48%> (-0.40%) ⬇️
cmd/run.go 85.71% <85.71%> (ø)
pkg/fileutil/files.go 78.51% <85.71%> (+2.16%) ⬆️
airflow/docker_image.go 83.40% <93.75%> (+6.93%) ⬆️
cmd/airflow.go 93.38% <100.00%> (ø)
cmd/root.go 96.82% <100.00%> (+0.05%) ⬆️

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.

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.

overall LGTM ❤️, this would add a lot of value for our users so thanks for all the effort. Have left a few minor nitpicks, let me know once those are addressed and the --task-logs logic has been added.

airflow/docker.go Outdated Show resolved Hide resolved
airflow/docker.go Show resolved Hide resolved
@@ -276,6 +276,91 @@ func (d *DockerImage) TagLocalImage(localImage string) error {
return nil
}

func (d *DockerImage) Run(dagID, envFile, settingsFile, containerName string, taskLogs bool) error {
// delete container
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// delete container

nit: is it still relevant, or else we can drop it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes we need to delete the container here just in case on exists and wasn't deleted before for some reason

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, but the comment over here does not fit any purpose to have it at the start of Run function, makes sense to have it where we are deleting the container but that is already there

airflow/docker_image.go Outdated Show resolved Hide resolved
settings/types.go Outdated Show resolved Hide resolved
settings/types.go Outdated Show resolved Hide resolved
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 👍 , but we would still need to fix the breaking unit tests before merging

@@ -276,6 +276,91 @@ func (d *DockerImage) TagLocalImage(localImage string) error {
return nil
}

func (d *DockerImage) Run(dagID, envFile, settingsFile, containerName string, taskLogs bool) error {
// delete container
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, but the comment over here does not fit any purpose to have it at the start of Run function, makes sense to have it where we are deleting the container but that is already there

@sunkickr
Copy link
Contributor Author

@neel-astro I moved the comment

Copy link
Contributor

@jemishp jemishp left a comment

Choose a reason for hiding this comment

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

looks good but need to bump coverage.

@sunkickr sunkickr merged commit 60b9873 into main Nov 21, 2022
@sunkickr sunkickr deleted the astro-test branch November 21, 2022 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants