Skip to content

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

Merged
devinrsmith merged 1 commit intodeephaven:mainfrom
devinrsmith:docker-registry-properties-check
Dec 13, 2021
Merged

Check the docker registry directories for gradle.properties.#1670
devinrsmith merged 1 commit intodeephaven:mainfrom
devinrsmith:docker-registry-properties-check

Conversation

@devinrsmith
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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