Add management api for continuous export of branches #844

arielshaqed · 2020-10-19T13:55:18Z

Part of #534.

- Remove a blank line - Override interfacer change that removes semantic information in exchange for a poorly-named irrelevant interface.

codecov-io · 2020-10-19T14:16:17Z

Codecov Report

Merging #844 into master will increase coverage by 2.10%.
The diff coverage is 76.13%.

@@            Coverage Diff             @@
##           master     #844      +/-   ##
==========================================
+ Coverage   42.90%   45.01%   +2.10%     
==========================================
  Files         135      140       +5     
  Lines       10571    10842     +271     
==========================================
+ Hits         4536     4881     +345     
+ Misses       5444     5330     -114     
- Partials      591      631      +40

Impacted Files	Coverage Δ
catalog/cataloger.go	`68.14% <ø> (ø)`
api/api_controller.go	`38.26% <66.66%> (+0.79%)`	⬆️
catalog/cataloger_export.go	`86.04% <86.04%> (ø)`
config/logger.go	`64.28% <0.00%> (-10.72%)`	⬇️
catalog/views.go	`97.01% <0.00%> (-1.65%)`	⬇️
stats/collector.go	`62.06% <0.00%> (-1.03%)`	⬇️
config/config.go	`33.55% <0.00%> (-0.23%)`	⬇️
catalog/diff.go	`18.51% <0.00%> (ø)`
auth/metadata.go	`0.00% <0.00%> (ø)`
catalog/validate.go	`94.44% <0.00%> (ø)`
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a51df8...4245f11. Read the comment docs.

arielshaqed · 2020-10-19T14:19:32Z

catalog/cataloger_export.go

+	if len(s) == 0 {
+		return nil, nil
+	}
+	regexpParts := strings.Split(s, "|")


Known issue (working on it while you review): This split is much too deep, e.g if you disjunct "a|b" and "c" it breaks.

Fixed this, got rid of all the de-disjunctioning.

It is not possible to roundtrip generating and unparsing disjunctions from the database: the regexp parser optimizes and rewrites portions of the regexp. So we need a copy of the input array stored in the database. (Generating a regexp disjunction back from this array will be easy, a small part of the deleted code did just that.)

Not part of the server API so untested. But may be used internally so needs testing.

guy-har

looks good

guy-har · 2020-10-21T11:11:12Z

api/api_controller.go

+
+		deps.LogAction("set_continuous_export")
+
+		if err != nil {


what err does this handle?

A long-gone err. Sorry. (But great flow analysis in the golang compiler)

guy-har · 2020-10-21T11:37:26Z

catalog/cataloger_export.go

+		if err = rows.StructScan(&rec); err != nil {
+			return nil, fmt.Errorf("scan configuration %+v: %w", rows, err)
+		}
+		fmt.Printf("[DEBUG] row %+v\n", rec)


should this still be here?

guy-har · 2020-10-21T11:58:23Z

docs/assets/js/swagger.yml

@@ -1258,54 +1287,6 @@ paths:
          schema:
            $ref: "#/definitions/error"

-  /repositories/{repository}/inventory/s3/import:


why was this removed?

This file is "auto-generated" by make build (in the sense of "blindly copied because Go and WebPack each have their own uniquely correct view of the world, but that view is hopelessly naïve and wrong so we have to hack our away around it because explicit dependencies are very 1990s". @nopcoder can explain why it is committed.
The last person to touch swagger.yml just forgot to build it.
Mine is an exact copy:

ariels@ariels:~/Dev/lakeFS$ diff swagger.yml docs/assets/js/swagger.yml

guy-har · 2020-10-21T12:01:39Z

ddl/000008_export_cfg.down.sql

@@ -0,0 +1 @@
+DROP TABLE catalog_branches_export;


I see we do that everywhere, so did it here too. Not sure it makes anything safer.

arielshaqed

Thanks; PTAL...

arielshaqed · 2020-10-21T14:09:33Z

api/api_controller.go

+
+		deps.LogAction("set_continuous_export")
+
+		if err != nil {


A long-gone err. Sorry. (But great flow analysis in the golang compiler)

arielshaqed · 2020-10-21T14:11:10Z

ddl/000008_export_cfg.down.sql

@@ -0,0 +1 @@
+DROP TABLE catalog_branches_export;


I see we do that everywhere, so did it here too. Not sure it makes anything safer.

arielshaqed · 2020-10-21T14:14:40Z

docs/assets/js/swagger.yml

@@ -1258,54 +1287,6 @@ paths:
          schema:
            $ref: "#/definitions/error"

-  /repositories/{repository}/inventory/s3/import:


This file is "auto-generated" by make build (in the sense of "blindly copied because Go and WebPack each have their own uniquely correct view of the world, but that view is hopelessly naïve and wrong so we have to hack our away around it because explicit dependencies are very 1990s". @nopcoder can explain why it is committed.
The last person to touch swagger.yml just forgot to build it.
Mine is an exact copy:

ariels@ariels:~/Dev/lakeFS$ diff swagger.yml docs/assets/js/swagger.yml

arielshaqed · 2020-10-21T14:15:16Z

catalog/cataloger_export.go

+		if err = rows.StructScan(&rec); err != nil {
+			return nil, fmt.Errorf("scan configuration %+v: %w", rows, err)
+		}
+		fmt.Printf("[DEBUG] row %+v\n", rec)


guy-har · 2020-10-21T14:35:10Z

LGTM

arielshaqed · 2020-10-22T06:57:54Z

Thanks!

arielshaqed added 2 commits October 19, 2020 16:52

Add DDL and cataloger functions configuring branch continuous export

a34dcf4

Configure branch continuous export: Swagger defs and API handlers

4032a01

arielshaqed requested a review from guy-har October 19, 2020 13:55

[checks] Format in accordance with golangci

56b0acf

- Remove a blank line - Override interfacer change that removes semantic information in exchange for a poorly-named irrelevant interface.

arielshaqed commented Oct 19, 2020

View reviewed changes

arielshaqed force-pushed the feature/534-management-api branch from 4d29c2e to 07afe46 Compare October 19, 2020 16:43

Test (and fix...) GetExportConfigurations

b8ca672

Not part of the server API so untested. But may be used internally so needs testing.

arielshaqed force-pushed the feature/534-management-api branch from 07afe46 to b8ca672 Compare October 19, 2020 20:40

guy-har approved these changes Oct 21, 2020

View reviewed changes

[CR] cleanups

4245f11

arielshaqed commented Oct 21, 2020

View reviewed changes

arielshaqed merged commit 7a8ca25 into master Oct 22, 2020

arielshaqed deleted the feature/534-management-api branch October 22, 2020 06:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add management api for continuous export of branches #844

Add management api for continuous export of branches #844

arielshaqed commented Oct 19, 2020

codecov-io commented Oct 19, 2020 •

edited

Loading

arielshaqed Oct 19, 2020

arielshaqed Oct 19, 2020

guy-har left a comment

guy-har Oct 21, 2020

arielshaqed Oct 21, 2020

guy-har Oct 21, 2020

arielshaqed Oct 21, 2020

guy-har Oct 21, 2020

arielshaqed Oct 21, 2020

guy-har Oct 21, 2020

arielshaqed Oct 21, 2020

arielshaqed left a comment

arielshaqed Oct 21, 2020

arielshaqed Oct 21, 2020

arielshaqed Oct 21, 2020

arielshaqed Oct 21, 2020

guy-har commented Oct 21, 2020

arielshaqed commented Oct 22, 2020

Add management api for continuous export of branches #844

Add management api for continuous export of branches #844

Conversation

arielshaqed commented Oct 19, 2020

codecov-io commented Oct 19, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guy-har left a 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

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

arielshaqed left a 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

guy-har commented Oct 21, 2020

arielshaqed commented Oct 22, 2020

codecov-io commented Oct 19, 2020 •

edited

Loading