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

Artemis Console next gen based on hawtIO 4 #5245

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

andytaylor
Copy link
Contributor

This is a PR to allow developers to build a variant of the latest Artemis Release with the new Artemis Console embedded, for testing. It will not currently merge and isnt intended to. It replaces #5149

I have staged the 1st Artemis Console release candidate, 1.0.0, and this build will use the staged dependency when built.

I will make this PR a draft and update it for each Artemis Release until it is decided to merge into main.

@clebertsuconic
Copy link
Contributor

This looks awesome Andy

I can't comment on the OpenID implementation as I don't have much context on what is what... but this looks very superior to the current console.. and the current console was not bad. Everything works quite fast.. it's amazing...

When can this be merged?

Copy link
Member

@gemmellr gemmellr left a comment

Choose a reason for hiding this comment

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

I havent had a chance to finish reviewing this in recent days, but I noticed various small issues when I looked mid week. There appear to be various regressions from issues already fixed in #5149, e.g those I did comment on, and others that look likely from peeking at the list of changed files (e.g old poms that were not removed, seem likely to be back again here)

Comment on lines +70 to +76
ssl.protocol = TLSv1.3
ssl.truststore = src/test/resources/hawtio.jks
ssl.truststorePassword = hawtio
ssl.keystore = src/test/resources/hawtio.jks
ssl.keystorePassword = hawtio
ssl.keyAlias = openid connect test provider
ssl.keyPassword = hawtio
Copy link
Member

Choose a reason for hiding this comment

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

Some of these seem like questionable out-the-box property values to be defining

Comment on lines +326 to +327
The licenses for dependencies sourced from NPM can be found in
licenses/NPMLicenses.txt
Copy link
Member

Choose a reason for hiding this comment

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

Does this file exist? Dont see it being added, or actions to otherwise source it.

@gemmellr
Copy link
Member

I pushed a couple of commits making similar changes as I had on the earlier PR, to fix some regressions since then.

That leaves the comments around the missing licence file being referenced, and the hawtio-oidc.properties file content (and/or it existing at all; didn't previously by default, maybe it still shouldn't?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants