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

feat: absolute imports #63

Merged
merged 17 commits into from Jan 24, 2020
Merged

feat: absolute imports #63

merged 17 commits into from Jan 24, 2020

Conversation

@sh0rez
Copy link
Collaborator

sh0rez commented Dec 15, 2019

jb now creates a directory structure inside of vendor/ that is similar to how go
does (github.com/grafana/jsonnet-libs). This is reflected in the final import
paths, which means they will be go-like.

This feature is enabled by default for new projects, older ones must enable it manually:

// jsonnetfile.json
{
  "legacyImports": true, // set to false to disable symlinks
  "dependencies": [
    ...
  ]
}

Fixes #61

@sh0rez sh0rez added the enhancement label Dec 15, 2019
@sh0rez sh0rez marked this pull request as ready for review Dec 15, 2019
@sh0rez sh0rez requested review from metalmatze and brancz Dec 15, 2019
@sh0rez sh0rez changed the title WIP feat: go-like import style feat: go-like import style Dec 15, 2019
spec/spec.go Show resolved Hide resolved
Copy link
Contributor

metalmatze left a comment

Very nice! Just one discussion I want to have about the changes in the jsonentfile.json

spec/spec.go Outdated Show resolved Hide resolved
@sh0rez sh0rez requested a review from metalmatze Dec 18, 2019
@brancz

This comment has been minimized.

Copy link
Contributor

brancz commented Dec 20, 2019

How does this behave when a downstream usage defines absolute imports but a dependency uses "legacy" imports? Would I break downstream and have to fix all dependencies before I can use it or would it work in a mixed state? (I'm not even sure which one I like better, I guess I'm just asking to make sure 🙂 )

@sh0rez

This comment has been minimized.

Copy link
Collaborator Author

sh0rez commented Dec 21, 2019

Huh, nice catch! Indeed, it would break all downstream dependencies ...

Either we live with this, or we symlink all files to their old location as well

@brancz

This comment has been minimized.

Copy link
Contributor

brancz commented Jan 6, 2020

Yeah I think symlinking should work, then we also don't need a config option I'd say, or rather one to opt out of the old behavior as opposed to opting into the new behavior.

sh0rez added 5 commits Dec 15, 2019
jb now creates a directory structure inside of vendor/ that is similar to how go
does (github.com/grafana/jsonnet-libs). This is reflected in the final import
paths, which means they will be go-like
Defaults to off, can be enabled in `jsonnetfile.json`
@sh0rez sh0rez force-pushed the sh0rez:go-import-style branch from 984db00 to 229aae4 Jan 8, 2020
sh0rez added 4 commits Jan 8, 2020
not an option anymore, will always do so and symlink
@sh0rez

This comment has been minimized.

Copy link
Collaborator Author

sh0rez commented Jan 8, 2020

@brancz implemented, you can take another look!

@brancz

This comment has been minimized.

Copy link
Contributor

brancz commented Jan 9, 2020

Looks like there are some consistently failing tests.

@sh0rez

This comment has been minimized.

Copy link
Collaborator Author

sh0rez commented Jan 9, 2020

Yup, will fix those later today, were passing locally yesterday tho

sh0rez added 2 commits Jan 9, 2020
It was possible to alias packages by changing `name` previously.

While names are now absolute (and computed), legacy links should still respect
old aliases to avoid breaking code.
@sh0rez sh0rez changed the title feat: go-like import style feat: absolute imports Jan 9, 2020
@sh0rez

This comment has been minimized.

Copy link
Collaborator Author

sh0rez commented Jan 9, 2020

@brancz Updated those, passing now.

Summary of the changes:

  • packages are now always installed into a go-like structure in vendor: github.com/user/repo/subdir
  • if legacyImports is set to true (default for existing projects), all packages are also linked into their old location
  • name is basically gone (imports are computed now), however it is still supported for legacy imports. If name and the computed name are equal, name is removed from the files

I tested those changes both on https://github.com/coreos/kube-prometheus and our internal Kubernetes config repository and it works!

@brancz

This comment has been minimized.

Copy link
Contributor

brancz commented Jan 10, 2020

My feeling is that for compatibility we should rather have legacy imports default to true and allow people that way to verify whether opting out still works for them. That would provide the smoothest migration experience I think.

@sh0rez

This comment has been minimized.

Copy link
Collaborator Author

sh0rez commented Jan 10, 2020

@brancz PTAL

sh0rez added 2 commits Jan 10, 2020
adds a command to automatically rewrite imports from legacy to absolute style
When a package was a prefix of another one, it broke.
Fixed that by using a proper regular expression. Added a test to make sure it
works as expected
@sh0rez

This comment has been minimized.

Copy link
Collaborator Author

sh0rez commented Jan 10, 2020

The tool for rewriting imports is pushed as well.

Usage:

# in the same directory as jsonnetfile.lock.json:
$ jb rewrite
@sh0rez sh0rez requested review from metalmatze and brancz and removed request for metalmatze and brancz Jan 20, 2020
@metalmatze

This comment has been minimized.

Copy link
Contributor

metalmatze commented Jan 20, 2020

Read through most of the code on my way home. Let me try to compile this branch and and then convert a few things with these updated paths.

Copy link
Contributor

brancz left a comment

Overall this looks really good, I'm curious though what would happen if we had two dependencies of the same repository, could they be imported individually? It doesn't seem so. I think it might have been something that was previously done (related to kubernetes-monitoring/kubernetes-mixin#331?). I'm a bit wary on putting that requirement on the community when we previously didn't but I think I'm ok with trying it and seeing how it goes.

cmd/jb/init.go Outdated Show resolved Hide resolved
cmd/jb/init.go Outdated Show resolved Hide resolved
@sh0rez

This comment has been minimized.

Copy link
Collaborator Author

sh0rez commented Jan 21, 2020

Overall this looks really good, I'm curious though what would happen if we had two dependencies of the same repository, could they be imported individually? It doesn't seem so.

Could you explain the issue a bit more? Don't get it yet

@brancz

This comment has been minimized.

Copy link
Contributor

brancz commented Jan 21, 2020

For example kube state metrics has one dir for its monitoring mixin and one for kube manifests. They are separately importable and versionable (and we actively do this). With these changes would that continue to work independently? RE: https://github.com/kubernetes/kube-state-metrics/tree/master/jsonnet

@sh0rez

This comment has been minimized.

Copy link
Collaborator Author

sh0rez commented Jan 21, 2020

I'd say yes, it should not change this.

If a repo has subdirs a, b and c and you install github.com/user/repo/subdir/a and github.com/user/repo/subdir/b you end up with a and b installed, but not c.

So it installs what's specified only, as before ... does this answer the question?

@brancz

This comment has been minimized.

Copy link
Contributor

brancz commented Jan 21, 2020

Ok awesome, I’ll give this a try on some repos and if that works as expected then I’m happy to merge.

@metalmatze

This comment has been minimized.

Copy link
Contributor

metalmatze commented Jan 22, 2020

Alright. I finally had some time to try check the new version of jsonnet-bundler:

├── grafana -> github.com/brancz/kubernetes-grafana/grafana
├── grafana-builder -> github.com/grafana/jsonnet-libs/grafana-builder
├── grafonnet -> github.com/grafana/grafonnet-lib/grafonnet
├── ksonnet -> github.com/ksonnet/ksonnet-lib
├── kube-prometheus -> kube-prometheus
- ├── kubernetes-mixin -> github.com/kubernetes-monitoring/kubernetes-mixin
├── node-mixin -> github.com/prometheus/node_exporter/docs/node-mixin
├── prometheus -> github.com/prometheus/prometheus/documentation/prometheus-mixin
├── prometheus-operator -> github.com/coreos/prometheus-operator/jsonnet/prometheus-operator
├── promgrafonnet -> github.com/kubernetes-monitoring/kubernetes-mixin/lib/promgrafonnet
└── slo-libsonnet -> github.com/metalmatze/slo-libsonnet/slo-libsonnet

The relative import for kube-prometheus seems to be broken with this branch. I am not totally sure how to move forward with that relative path feature, once we have absolute import paths. We need to symlink absolute paths to a relative still, I guess.

@sh0rez

This comment has been minimized.

Copy link
Collaborator Author

sh0rez commented Jan 22, 2020

Alright. I finally had some time to try check the new version of jsonnet-bundler:

├── grafana -> github.com/brancz/kubernetes-grafana/grafana
├── grafana-builder -> github.com/grafana/jsonnet-libs/grafana-builder
├── grafonnet -> github.com/grafana/grafonnet-lib/grafonnet
├── ksonnet -> github.com/ksonnet/ksonnet-lib
├── kube-prometheus -> kube-prometheus
- ├── kubernetes-mixin -> github.com/kubernetes-monitoring/kubernetes-mixin
├── node-mixin -> github.com/prometheus/node_exporter/docs/node-mixin
├── prometheus -> github.com/prometheus/prometheus/documentation/prometheus-mixin
├── prometheus-operator -> github.com/coreos/prometheus-operator/jsonnet/prometheus-operator
├── promgrafonnet -> github.com/kubernetes-monitoring/kubernetes-mixin/lib/promgrafonnet
└── slo-libsonnet -> github.com/metalmatze/slo-libsonnet/slo-libsonnet

The relative import for kube-prometheus seems to be broken with this branch. I am not totally sure how to move forward with that relative path feature, once we have absolute import paths. We need to symlink absolute paths to a relative still, I guess.

How can I reproduce this?

@metalmatze

This comment has been minimized.

Copy link
Contributor

metalmatze commented Jan 22, 2020

Clone the kube-prometheus repository and then run jb install in the root with your binary.

They actually still use the old style, which is fine. LegacyLinking
messed them up, but from now on it just ignores symlinks that match a localPackage.
@sh0rez

This comment has been minimized.

Copy link
Collaborator Author

sh0rez commented Jan 22, 2020

Clone the kube-prometheus repository and then run jb install in the root with your binary.

@metalmatze This issue is now resolved. The legacyLink code was not aware of local packages being symlinks as well and overwrote those with nonsense. It is now properly ignoring them, so at least in my local testing there are no broken symlinks anymore!

Copy link
Contributor

metalmatze left a comment

I've tried this in a couple of projects. Works as expected now.
Really awesome enhancement! 👍 🎉
LGTM

@sh0rez sh0rez merged commit 7b8a783 into jsonnet-bundler:master Jan 24, 2020
1 check passed
1 check passed
continuous-integration/drone/pr Build is passing
Details
@sh0rez sh0rez deleted the sh0rez:go-import-style branch Jan 24, 2020
@hangxie

This comment has been minimized.

Copy link
Contributor

hangxie commented Jan 24, 2020

Can we cut a new release? I think this is a major change and worth a release.

@sh0rez

This comment has been minimized.

Copy link
Collaborator Author

sh0rez commented Jan 24, 2020

Can we cut a new release? I think this is a major change and worth a release.

Because this is a major change, I would prefer to leave it on master only for a week or so to catch possible bugs / issues before we release

@hangxie

This comment has been minimized.

Copy link
Contributor

hangxie commented Jan 24, 2020

leave it on master only for a week or so to catch possible bugs / issues before we release

SGTM

@metalmatze

This comment has been minimized.

Copy link
Contributor

metalmatze commented Jan 24, 2020

Hurray!
Yes, I agree lets give it some time, but starting to migrate a few projects and then we can cut a release for sure! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.