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

Check the docker registry directories for gradle.properties. #1670

Merged

Conversation

devinrsmith
Copy link
Member

Otherwise, project may become unbuildable if you check out a branch that has a new registry, and then checkout a branch that doesn't have that registry.

Otherwise, project may become unbuildable if you check out a branch that has a new registry, and then checkout a branch that doesn't have that registry.
@niloc132
Copy link
Member

Two thoughts:

  • If this is a good idea, why don't we apply for all projects across the repo? (and while we're at it, the standard project naming convention is "foo:bar" not "foo-bar", for a foo/bar/*.gradle, right? why do we break that convention?) The fact that it isn't suggests to me that we should think very carefully about implementing this, like worrying about local file(s) auto creating new projects that we werent expecting - forcing the user to edit setting.gradle (and sometimes root build.gradle).
  • Which brings me to the next point - the root build.gradle is growing a lot with each new project, and the addition of modsAreDockerRegistry to subtract out projects that are uninteresting for some other uses seems to add to the problem. Is there no better way to do this, to opt-in rather than opt-out? esp since these projects at this time all have the common feature of being in the docker-registry project? :proto started off this way, but then didn't work because we actually only have one real proto project, the other two don't run protoc at all.

@devinrsmith
Copy link
Member Author

I'm not against making the project structure better, but in the immediate term, this is a strict improvement to what already exists IMO (as we saw, Charles ran into an issues that would have been solved if he had this patch). We can get into conventions discussions foo:bar vs foo-bar, opt-in vs opt-out, etc... but I don't want that to hold up a useful fix.

@devinrsmith devinrsmith merged commit da998f2 into deephaven:main Dec 13, 2021
@devinrsmith devinrsmith deleted the docker-registry-properties-check branch December 13, 2021 17:07
@devinrsmith
Copy link
Member Author

Added discussion: #1684

@github-actions github-actions bot locked and limited conversation to collaborators Dec 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants