New Registry Commands #1148

fritz-astronomer · 2023-04-11T05:15:59Z

Description

astro registry dag add dbt-basic --version 1.0.4
- add a DAG file (just the .py) directly from the registry, at the filepath defined by the registry, optionally add any providers.
astro registry provider add snowflake
- add the provider to requirements.txt
~~astro registry dev init https://github.com/astronomer/my-project/~~
- ~~init a project from the registry~~

🧪 Functional Testing + 📸 Screenshots

List the functional testing steps to confirm this feature or fix.
Add screenshots to illustrate the validity of these changes.

registry_demo.mov

issue:

Import astronomer registry dags through astro cli #720

📋 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

fritz-astronomer · 2023-04-11T14:12:36Z

First few commands complete. Want to add a few more quality-of-life things, and init

cmd/registry/root.go

iancmoritz

Fritz - this is fantastic. Super small changes overall but thank you!

cmd/registry/root.go

iancmoritz · 2023-04-11T20:05:01Z

cmd/registry/root.go

+	return getProviderUrl.String()
+}
+
+func getProviderRoute(providerId string, providerVersion string) string {


Suggested change

func getProviderRoute(providerId string, providerVersion string) string {

func getProviderRoute(providerName string, providerVersion string) string {

I was considering apache-airflow-provider-snowflake an id and snowflake a name. And this one specifically takes the id not the name.

The airflow schema refers to apache-airflow-provider-snowflake as a "provider package name" and amazon would be the "provider name". see here: https://github.com/apache/airflow/blob/main/airflow/provider_info.schema.json

iancmoritz · 2023-04-11T20:05:21Z

cmd/registry/root.go

+	getProviderUrl := url.URL{
+		Scheme: "https",
+		Host:   registryHost,
+		Path:   fmt.Sprintf("%s/%s", registryApi, fmt.Sprintf(providerRoute, providerId, providerVersion)),


Suggested change

Path: fmt.Sprintf("%s/%s", registryApi, fmt.Sprintf(providerRoute, providerId, providerVersion)),

Path: fmt.Sprintf("%s/%s", registryApi, fmt.Sprintf(providerRoute, providerName, providerVersion)),

fritz-astronomer · 2023-05-17T22:09:45Z

Feedback notes:

Linting, add test coverage
command defs then functions
cmd.cloud.deployment_test, cloud.deployment_test.go
pkg.util.go
astro registry <dag|provider> <add|list>

jwitz

Some minor copy review

cmd/registry/root.go

fritz-astronomer · 2023-06-08T17:53:02Z

Future work:

it'd be awesome if the registry had example dags for each provider - even just a naming convention. coughcough https://github.com/astronomer/module_quickstart/blob/snowflake/dags/snowflake/example_snowflake.py coughcough
it'd be awesome if the registry had package.txt additions
Gotta figure out the registry init thing and unpacking the .zip in place
dags list, providers list

Co-authored-by: Ian Moritz <ian@alkeri.com>

Co-authored-by: Jake Witz <74574233+jwitz@users.noreply.github.com>

codecov · 2023-06-20T14:56:43Z

Codecov Report

Patch coverage: 68.94% and project coverage change: -0.23 ⚠️

Comparison is base (9effcb1) 87.51% compared to head (8041598) 87.28%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1148      +/-   ##
==========================================
- Coverage   87.51%   87.28%   -0.23%     
==========================================
  Files         110      113       +3     
  Lines       12999    13160     +161     
==========================================
+ Hits        11376    11487     +111     
- Misses        949      986      +37     
- Partials      674      687      +13

Impacted Files	Coverage Δ
pkg/fileutil/files.go	`76.92% <0.00%> (-2.45%)`	⬇️
cmd/registry/dag.go	`64.28% <64.28%> (ø)`
cmd/registry/provider.go	`72.94% <72.94%> (ø)`
cmd/registry/root.go	`76.92% <76.92%> (ø)`
cmd/root.go	`91.42% <100.00%> (+0.38%)`	⬆️

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

neel-astro

left some minor comments around simplifying the logic to improve readability, LGTM otherwise

cmd/registry/dag_test.go

cmd/registry/provider.go

pkg/util/util.go

pkg/httputil/http.go

cmd/registry/provider.go

cmd/registry/provider_test.go

pkg/httputil/http.go

Co-authored-by: Neel Dalsania <neel.dalsania@astronomer.io>

for more information, see https://pre-commit.ci

neel-astro

Great work and thanks for the contribution, this would be quite useful for users. Code LGTM, has left some final nits to squash.

cmd/registry/dag_test.go

neel-astro · 2023-06-21T06:34:31Z

cmd/registry/provider.go

+	if err != nil {
+		log.Fatal(err)
+	}
+


Suggested change

defer f.Close()

nit: file close should be part of defer so that it is called from any return point in the function. Sorry I might have missed it in my earlier review

The linter didn't like this because there are log.Fatal's below. I also unfortunately can't run the linter locally for some reason, so that wasn't fun to find a configuration it was happy with, beyond what you see here. Does the line in this location pass the linter for you?

ideally, we shouldn't have log.Fatal(), but rather have it as log.Error() and then return the error from the function, and then the caller should handle the error and act accordingly.

The linter is complaining because as of now f.Close() even if in defer won't be called if there is any log.Fatal() after that in the code execution path. It's not a big issue if we are just missing out on f.Close(), but ideally in Golang we don't exit abruptly and handle errors everywhere, which makes it simpler to maintain in the future.

that makes sense. I'll see if I can address that

neel-astro · 2023-06-21T06:34:53Z

cmd/registry/provider.go

+		}
+		_, _ = fmt.Fprintf(out, "\nWrote %s to %s", providerString, filename)
+	}
+	_ = f.Close()


Suggested change

_ = f.Close()

nit: not needed once we have it as defer

cmd/registry/provider_test.go

Co-authored-by: Neel Dalsania <neel.dalsania@astronomer.io>

kushalmalani · 2023-06-21T17:18:06Z

cmd/registry/dag.go

+	filledDagRoute := fmt.Sprintf(dagRoute, dagID, dagVersion)
+	getDagURL := url.URL{
+		Scheme: "https",
+		Host:   registryHost,


Would this work only on prod env?

didn't take a dev registry into account yet

kushalmalani · 2023-06-21T17:30:36Z

@fritz-astronomer Do these commands need to run in an airflow project? If yes, we need to add that check. Something we do for astro deploy

fritz-astronomer · 2023-06-21T17:53:26Z

@kushalmalani - they don't actually need to. They just add an entry to a requirements.txt or download a file to a dags folder. I think in both instances the folder or file is created if it doesn't exist.

This reverts commit beaae88.

This reverts commit 0b4c617.

fritz-astronomer mentioned this pull request Apr 11, 2023

Import astronomer registry dags through astro cli #720

Closed

1 task

iancmoritz reviewed Apr 11, 2023

View reviewed changes

cmd/registry/root.go Outdated Show resolved Hide resolved

iancmoritz suggested changes Apr 11, 2023

View reviewed changes

sunkickr requested a review from jwitz April 21, 2023 15:25

jwitz reviewed May 24, 2023

View reviewed changes

fritz-astronomer force-pushed the registry branch from 0a1b108 to 71bce22 Compare June 9, 2023 05:08

fritz-astronomer marked this pull request as ready for review June 9, 2023 05:08

fritz-astronomer requested review from neel-astro, kushalmalani and sunkickr as code owners June 9, 2023 05:08

fritz-astronomer requested review from iancmoritz, odaneau-astro and jwitz June 9, 2023 05:08

fritz-astronomer and others added 8 commits June 9, 2023 01:09

registry add dag/provider, skeleton for registry init

cdabc63

Update cmd/registry/root.go

880afc8

Co-authored-by: Ian Moritz <ian@alkeri.com>

Update cmd/registry/root.go

39d2b9d

Co-authored-by: Ian Moritz <ian@alkeri.com>

minor edit; PR feedback

cf3dbaf

Update cmd/registry/root.go

0d1d9c2

Co-authored-by: Jake Witz <74574233+jwitz@users.noreply.github.com>

Apply suggestions from code review

9e625e5

Co-authored-by: Jake Witz <74574233+jwitz@users.noreply.github.com>

start of tests, PR feedback on code structure

bbf5b61

get rid of unfinished code, happy path tests, refactors

acf48e2

fritz-astronomer force-pushed the registry branch from 71bce22 to acf48e2 Compare June 9, 2023 05:10

fix linter issues

aa74304

fritz-astronomer force-pushed the registry branch from a345133 to aa74304 Compare June 9, 2023 05:55

Merge branch 'main' into registry

919c318

update copy

89c8689

neel-astro reviewed Jun 20, 2023

View reviewed changes

fritz-astronomer and others added 7 commits June 20, 2023 17:21

Apply suggestions from code review

fbb07bb

Co-authored-by: Neel Dalsania <neel.dalsania@astronomer.io>

[pre-commit.ci] auto fixes from pre-commit.com hooks

250134b

for more information, see https://pre-commit.ci

PR feedback

883828c

fix linting errors

003be4e

fix linting errors

5453468

fix linting errors

1b3fc10

fix linting errors

44be44e

neel-astro approved these changes Jun 21, 2023

View reviewed changes

Apply suggestions from code review

0b4c617

Co-authored-by: Neel Dalsania <neel.dalsania@astronomer.io>

kushalmalani reviewed Jun 21, 2023

View reviewed changes

fritz-astronomer added 3 commits June 21, 2023 13:57

attempt to sort imports

beaae88

Revert "attempt to sort imports"

f9a3eab

This reverts commit beaae88.

Revert "Apply suggestions from code review"

8041598

This reverts commit 0b4c617.

kushalmalani approved these changes Jun 26, 2023

View reviewed changes

sunkickr approved these changes Jun 29, 2023

View reviewed changes

sunkickr merged commit cd1b3b4 into main Jun 29, 2023

sunkickr deleted the registry branch June 29, 2023 15:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Registry Commands #1148

New Registry Commands #1148

fritz-astronomer commented Apr 11, 2023 •

edited

Loading

fritz-astronomer commented Apr 11, 2023

iancmoritz left a comment

iancmoritz Apr 11, 2023

fritz-astronomer Apr 12, 2023

odaneau-astro Apr 12, 2023 •

edited

Loading

iancmoritz Apr 11, 2023

fritz-astronomer commented May 17, 2023

jwitz left a comment

fritz-astronomer commented Jun 8, 2023 •

edited

Loading

codecov bot commented Jun 20, 2023 •

edited

Loading

neel-astro left a comment

neel-astro left a comment

neel-astro Jun 21, 2023

fritz-astronomer Jun 21, 2023 •

edited

Loading

neel-astro Jun 26, 2023

fritz-astronomer Jun 27, 2023

neel-astro Jun 21, 2023

kushalmalani Jun 21, 2023

fritz-astronomer Jun 27, 2023

kushalmalani commented Jun 21, 2023 •

edited

Loading

fritz-astronomer commented Jun 21, 2023

	func getProviderRoute(providerId string, providerVersion string) string {
	func getProviderRoute(providerName string, providerVersion string) string {

	Path: fmt.Sprintf("%s/%s", registryApi, fmt.Sprintf(providerRoute, providerId, providerVersion)),
	Path: fmt.Sprintf("%s/%s", registryApi, fmt.Sprintf(providerRoute, providerName, providerVersion)),

New Registry Commands #1148

New Registry Commands #1148

Conversation

fritz-astronomer commented Apr 11, 2023 • edited Loading

Description

🧪 Functional Testing + 📸 Screenshots

📋 Checklist

fritz-astronomer commented Apr 11, 2023

iancmoritz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odaneau-astro Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fritz-astronomer commented May 17, 2023

jwitz left a comment

Choose a reason for hiding this comment

fritz-astronomer commented Jun 8, 2023 • edited Loading

codecov bot commented Jun 20, 2023 • edited Loading

Codecov Report

neel-astro left a comment

Choose a reason for hiding this comment

neel-astro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fritz-astronomer Jun 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kushalmalani commented Jun 21, 2023 • edited Loading

fritz-astronomer commented Jun 21, 2023

fritz-astronomer commented Apr 11, 2023 •

edited

Loading

odaneau-astro Apr 12, 2023 •

edited

Loading

fritz-astronomer commented Jun 8, 2023 •

edited

Loading

codecov bot commented Jun 20, 2023 •

edited

Loading

fritz-astronomer Jun 21, 2023 •

edited

Loading

kushalmalani commented Jun 21, 2023 •

edited

Loading