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

Delta Lake Diff: Change default plugin loading #5495

Merged

Conversation

Jonathan-Rosenberg
Copy link
Contributor

Change Description

First, look up the default directory for an executable named delta and register it if it's there.
Then test the different diff plugin configurations available and run over the previous registration if needed.

Why not the other way around?
Because it's easier and doesn't cost too much...

@Jonathan-Rosenberg Jonathan-Rosenberg added exclude-changelog PR description should not be included in next release changelog minor-change Used for PRs that don't require issue attached labels Mar 15, 2023
Copy link
Contributor

@arielshaqed arielshaqed left a comment

Choose a reason for hiding this comment

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

Nice!

@@ -326,8 +332,8 @@ type Config struct {
Code string `mapstructure:"code"`
} `mapstructure:"snippets"`
} `mapstructure:"ui"`
Diff map[string]DiffProps `mapstructure:"diff"`
Plugins map[string]PluginProps `mapstructure:"plugins"`
Diff map[string]DiffProps `mapstructure:"diff"`
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should define the supported formats and not leave it dynamic as it is not dynamic.
If the code was able to support new formats dynamically I would keep the map. But not it looks like we just moved the lookup at runtime to the specific format we support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created an issue for that and will be handled in the following iteration.

logging.Default().Errorf("failed to register a plugin for an invalid path: '%s'", props.Path)
continue
}
pluginPath = pp
if props.Version != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove the Version pointer from the configuration as we fallback to version 1 in the code - we can document that 0 is the default.

pkg/plugins/diff/service.go Outdated Show resolved Hide resolved
Jonathan-Rosenberg and others added 2 commits March 19, 2023 12:16
Co-authored-by: Barak Amar <barak.amar@treeverse.io>
* Display images in markdown with support for lakefs:// URIs (#5449)

* Display images in markdown with support for lakefs:// URIs

* change dockerfile to include delta diff

* change Dockerfile to include the Delta Lake diff plugin

* fix docker-compose files

* add a stage with both lakefs and plugins and separate it from a lakefs-only stage

* PR fixes

* Delta Lake Diff: Enable feature (#5508)

* fix hover issue when hovering over a tree-entry-row

* enable Delta Lake diff feature

* remove experimental delta diff button

* PR fixes

* change docker image publish target to lakefs-plugins

* fix bug hunt bugs (#5534)

* remove empty line

* Delta Lake: Update Dockerfile base images to Alpine (#5546)

* change the Dockerfile back to Alpine and build the Delta plugin dynamically

* remove alpine-sdk from dockerfile

* add alpine-sdk

* add comment about the 'RUSTFLAGS=-Ctarget-feature=-crt-static' flag

* change back from bullseye to alpine

---------

Co-authored-by: eladlachmi <110764839+eladlachmi@users.noreply.github.com>
@Jonathan-Rosenberg Jonathan-Rosenberg merged commit 39b9aa1 into feature/delta/releaser Mar 22, 2023
@Jonathan-Rosenberg Jonathan-Rosenberg deleted the feature/delta/change-default-loading branch March 22, 2023 09:27
Jonathan-Rosenberg added a commit that referenced this pull request Mar 22, 2023
* add makefile target to build delta diff plugin

* test goreleaser  with rust

* remove cache step

* add activate on push to this branch

* change working-directory

* remove working-directory

* remove target from args

* add protoc for mac and name steps

* add version to protoc action

* switch from Arduino

* use correct version

* change back to arduino

* change version to 3.x

* add protoc to unix and windows

* change version to 3.x

* change to release, upload artifact

* downloading artifacts

* add needs to releaser

* add build

* move --release to arguments

* fix artifact location

* fix naming

* add cache

* add .exe suffix to windows build target

* add id to the download artifact step

* add registry and index to cache

* add renaming of artifacts

* move download-artifacts to the goreleaser job

* change cache support to use custom action

* run goreleaser without truly releasing

* fix goreleaser

* change delta artifact location

* fix releaser yaml

* change releaser to be production ready

* change from 3 jobs definitions to a single job definition with a more complex matrix

* fix makefile

* Delta Lake Diff: Change default plugin loading (#5495)

* change default behavior of plugin loading to scan in the default plugins directory for known plugins

* get delta lake diff plugin from default path if available

* add homedir.Expand call to custom plugin path

* remove empty line

Co-authored-by: Barak Amar <barak.amar@treeverse.io>

* change version from a pointer to an int

* Delta Lake Diff: Dockerfile (#5496)

* Display images in markdown with support for lakefs:// URIs (#5449)

* Display images in markdown with support for lakefs:// URIs

* change dockerfile to include delta diff

* change Dockerfile to include the Delta Lake diff plugin

* fix docker-compose files

* add a stage with both lakefs and plugins and separate it from a lakefs-only stage

* PR fixes

* Delta Lake Diff: Enable feature (#5508)

* fix hover issue when hovering over a tree-entry-row

* enable Delta Lake diff feature

* remove experimental delta diff button

* PR fixes

* change docker image publish target to lakefs-plugins

* fix bug hunt bugs (#5534)

* remove empty line

* Delta Lake: Update Dockerfile base images to Alpine (#5546)

* change the Dockerfile back to Alpine and build the Delta plugin dynamically

* remove alpine-sdk from dockerfile

* add alpine-sdk

* add comment about the 'RUSTFLAGS=-Ctarget-feature=-crt-static' flag

* change back from bullseye to alpine

---------

Co-authored-by: eladlachmi <110764839+eladlachmi@users.noreply.github.com>

---------

Co-authored-by: Barak Amar <barak.amar@treeverse.io>
Co-authored-by: eladlachmi <110764839+eladlachmi@users.noreply.github.com>

---------

Co-authored-by: Barak Amar <barak.amar@treeverse.io>
Co-authored-by: eladlachmi <110764839+eladlachmi@users.noreply.github.com>
nopcoder added a commit that referenced this pull request Apr 17, 2023
* add makefile target to build delta diff plugin

* test goreleaser  with rust

* remove cache step

* add activate on push to this branch

* change working-directory

* remove working-directory

* remove target from args

* add protoc for mac and name steps

* add version to protoc action

* switch from Arduino

* use correct version

* change back to arduino

* change version to 3.x

* add protoc to unix and windows

* change version to 3.x

* change to release, upload artifact

* downloading artifacts

* add needs to releaser

* add build

* move --release to arguments

* fix artifact location

* fix naming

* add cache

* add .exe suffix to windows build target

* add id to the download artifact step

* add registry and index to cache

* add renaming of artifacts

* move download-artifacts to the goreleaser job

* change cache support to use custom action

* run goreleaser without truly releasing

* fix goreleaser

* change delta artifact location

* fix releaser yaml

* change releaser to be production ready

* change from 3 jobs definitions to a single job definition with a more complex matrix

* fix makefile

* Delta Lake Diff: Change default plugin loading (#5495)

* change default behavior of plugin loading to scan in the default plugins directory for known plugins

* get delta lake diff plugin from default path if available

* add homedir.Expand call to custom plugin path

* remove empty line

Co-authored-by: Barak Amar <barak.amar@treeverse.io>

* change version from a pointer to an int

* Delta Lake Diff: Dockerfile (#5496)

* Display images in markdown with support for lakefs:// URIs (#5449)

* Display images in markdown with support for lakefs:// URIs

* change dockerfile to include delta diff

* change Dockerfile to include the Delta Lake diff plugin

* fix docker-compose files

* add a stage with both lakefs and plugins and separate it from a lakefs-only stage

* PR fixes

* Delta Lake Diff: Enable feature (#5508)

* fix hover issue when hovering over a tree-entry-row

* enable Delta Lake diff feature

* remove experimental delta diff button

* PR fixes

* change docker image publish target to lakefs-plugins

* fix bug hunt bugs (#5534)

* remove empty line

* Delta Lake: Update Dockerfile base images to Alpine (#5546)

* change the Dockerfile back to Alpine and build the Delta plugin dynamically

* remove alpine-sdk from dockerfile

* add alpine-sdk

* add comment about the 'RUSTFLAGS=-Ctarget-feature=-crt-static' flag

* change back from bullseye to alpine

---------

Co-authored-by: eladlachmi <110764839+eladlachmi@users.noreply.github.com>

---------

Co-authored-by: Barak Amar <barak.amar@treeverse.io>
Co-authored-by: eladlachmi <110764839+eladlachmi@users.noreply.github.com>

---------

Co-authored-by: Barak Amar <barak.amar@treeverse.io>
Co-authored-by: eladlachmi <110764839+eladlachmi@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exclude-changelog PR description should not be included in next release changelog minor-change Used for PRs that don't require issue attached
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants