Add a theme module by moving/reusing existing gradle code

[bickelj]

The result (and means) of this new module:

• Gradle at the top-level (auth) (existing code moved up from twilio-keycloak-provider).
• A twilio-keycloak-provider module with its own build script (existing code moved up as well).
• A pdc-keycloak-theme module with its own build script (new).
• An expected directory structure for pdc-keycloak-theme (new, the structure follows what the build script expects).

To build a submodule, cd [module name] and then ../gradlew [task].

Keycloak is Apache License 2.0 so the theme keeps that license. The SMS Authenticator was originally Expat (MIT) so it keeps Expat.

Issue #4 Set up CI Issue #7 Theme the user-facing Keycloak pages

[jasonaowen]

Also, I don't know how much feedback on git commit structure you want, but I think each of the bullet points in your PR description would make for a good, focused, individual commit!

[bickelj]

Glad to see CI running!

Yea!

Looking at the GitHub workflow output, I think it might make sense to add the gradle build action to speed up builds and reduce spam (like the "what's new in this version" on first installation, which I believe will be every installation, because CI).

Yes, that will help. The focus of this PR was really issue #7 (add a directory structure for a new theme) and only incidentally issue #4 (changes to build scripts, let that new directory structure be easily built). I had a note in #4 to re-arrange the directory structure and #7 gave the impetus.

[bickelj]

Also, I don't know how much feedback on git commit structure you want, but I think each of the bullet points in your PR description would make for a good, focused, individual commit!

It will be an ongoing discussion, I suppose. I like a good atomic commit, but then we end up squashing them anyway, right? Do we want 50 reviews and 50 merges for 50 minor changes?

I think the max commits here would be two as well, one to fix the directory structure of the existing module and one to add the directory structure for the new module. But again, the impetus is the same: a new module highlighted the need for the change. So I think it's OK to keep in one.

[jasonaowen]

I generally do not like to squash commits as part of merging! Atomic commits avoid that need. In a recent PR there were a bunch of fixup commits and commits that implemented review suggestions, and when that workflow is followed then squashing is better, but the approach I take and encourage others to take is to immediately rebase with --autosquash after making a fixup commit, and force push the branch - that way the PR is always in a ready-to-merge form!

[kfogel]

FWIW, @bickelj and @jasonaowen, my general feeling is that squashing commits during the PR development/review phase is fine (it goes along with rebasing), whereas squashing commits on merge is not. After all, the whole point of a finished PR is that everyone agrees that what's getting merged is the right thing -- not just in terms of the line-by-line code changes, but in terms of the commit message(s) and commit(s) too. That is, by the end of review, the commit structure is oriented toward promoting human understanding of what this changeset is all about.

So that means squash/rebase to our hearts' content when developing the PR, but then don't squash on merge: what we merge is what was reviewed.

[kfogel]

(I think I might just be re-articulating what @jasonaowen says above.)

[kfogel]

And if y'all agree, feel free to submit a change to the meta repository documenting this :-).

[bickelj]

@kfogel That all makes sense. @jasonaowen Bringing it back to this particular change, perhaps it is the messaging in the PR or commit that needs work rather than splitting it into two or four commits. The goal was simple: add a skeleton for the theme directory. To that end, the existing build code was re-used by being moved rather than adding duplicative code. Perhaps some of the bullet points make it sound like all of the things are new when they are really not?

I have edited the PR description a bit in hopes that it clarifies what is going on. And I can see the case for two commits rather than one but I still cannot see the case for four commits.

[jasonaowen]

Yes, two commits sounds quite reasonable! My general rule of thumb is whenever someone (including myself...) makes a list of things included in a PR, there should probably be at least that many commits in the PR, but I should have looked more closely at this particular list before making that suggestion.

[jasonaowen]

... add the gradle build action to speed up builds and reduce spam...

Yes, that will help. The focus of this PR was really...

It also sounds like you want to defer more CI work to a separate, later PR, and that's fine with me!

[bickelj]

I created a new issue #10 briefly describing the problem and put commit number one in new PR #11. This is the first of two PRs that will supersede this PR.