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.

Sign up for GitHub

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

Jump to bottom

Move to kebab case and add categories in model #4851

Merged
merged 15 commits into from
Oct 28, 2019
Merged

Move to kebab case and add categories in model #4851

merged 15 commits into from
Oct 28, 2019

Conversation

maxandersen
Copy link
Member

@maxandersen maxandersen commented Oct 24, 2019 •
edited
Loading

Why:

  • all other formatting in quarkus uses kebab-case
  • categories needed in registry

This change addreses the need by:

  • use kebab property naming for parsing of artifacts class.
  • rename shortName to short-name and put in metadata
  • add tests
  • add categories
  • move to use jackson parsing in preparation for move to yaml (I think we'll do that in a separate PR)

@maxandersen maxandersen force-pushed the kebabcase_and_categories branch 4 times, most recently from 011adec to 60ba700 Compare October 27, 2019 06:48
maxandersen
maxandersen commented Oct 27, 2019
View reviewed changes
devtools/bom-descriptor-json/src/main/resources/extensions-overrides.json Outdated
@@ -0,0 +1,31 @@

Copy link
Member Author

Choose a reason for hiding this comment

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

Just draft example - will be removed before final review/merge.

@maxandersen
Copy link
Member Author

@aloubyansky @ia3andy I would like to get review/confirmation on the general idea in this pr - majority is just updating the actual .json files; actual code is fairly small. It basically just clean up the descriptor structure and introduces notion of categories in platform descriptor.

Once we merge this I'll do a follow up that moves the quarkus-descriptor.json to quarkus-extension.yaml - that one is a bit trickier because we have spread out reading of this file so would rather do that change in isolation.

@maxandersen maxandersen force-pushed the kebabcase_and_categories branch from 60ba700 to 832c09e Compare October 27, 2019 07:00
aloubyansky
aloubyansky requested changes Oct 27, 2019
View reviewed changes
independent-projects/bootstrap/maven-plugin/pom.xml Outdated
@@ -1,74 +1,78 @@
<project xmlns="http://maven.apache.org/POM/4.0.0"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
Copy link
Member

Choose a reason for hiding this comment

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

Could you please undo the formatting here?

Copy link
Member

Choose a reason for hiding this comment

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

And this one?

aloubyansky and others added 9 commits October 27, 2019 21:26
@aloubyansky @maxandersen
Removed the legacy extensions.json and enahanced the platform json de…
738f0d4 
…scriptor loader to load the default version from the classpath
@maxandersen
Move to kebab case and add categories in model
d8f7f2d 
Why:

 * all other formatting in quarkus uses kebab-case
 * categories needed in registry

This change addreses the need by:

 * use kebab property naming for parsing of artifacts class.
 * rename shortName to short-name
 * add tests
 * add categories
@maxandersen
workaround new unitests limits
0fbcf50
@maxandersen
adjust from latest rebases/feature add
3e508d3
@maxandersen
remove dummy tests
4eee942
@maxandersen
avoid NPE in case of no keywords
76fc7b9
@maxandersen
move to use only jackson parser for ExDescriptorMojo
ea810d0
@maxandersen
add updated quarkus-extension.json for all extensions
4392467
@maxandersen
format one single file
dd4655f
@maxandersen maxandersen force-pushed the kebabcase_and_categories branch from 3746ca7 to 3d61c30 Compare October 27, 2019 20:46
@maxandersen maxandersen mentioned this pull request Oct 27, 2019
Open
@maxandersen
Copy link
Member Author

Note: reason for the whitespace diff in ..json files is that we moved to use jackson for the read/write and it uses " : " rather than ": " as used by eclipsesource json. didn't find a flag to control that and since trying to move to yaml anyways I didn't want to spend unnecssary amount of time tweaking the pretty print of json.

@maxandersen maxandersen changed the title WIP: Move to kebab case and add categories in model Move to kebab case and add categories in model Oct 27, 2019
@maxandersen
Copy link
Member Author

I got no idea why the builds are now failing with vm errors - this is similar kind of error I saw in master before I did the changes in this PR.

@maxandersen
Copy link
Member Author

now actually seeing relevant test errors....investigating!

@aloubyansky
Copy link
Member

I approved before checking the CI. There are NPEs in the tests though.

@maxandersen
Copy link
Member Author

I approved before checking the CI. There are NPEs in the tests though.

yes - just saw them. looking.

@maxandersen
Copy link
Member Author

ok, shortName failing null check - that was a change I did last so must have not done full IT test on that. fixing.

@maxandersen maxandersen marked this pull request as ready for review October 27, 2019 23:42
@maxandersen
Copy link
Member Author

okey, that was easy fix - pushed - lets see what the tests says this time ;)

@maxandersen
Copy link
Member Author

all test passed, but then noticed the dummy extensions-override.json was still there - so now removed but I expect the tests to still pass and we should push then.

but not going to wait for that to complete before I go sleep - so if someone sees this be green before I wake tomorrow then please merge ;)

ia3andy
ia3andy approved these changes Oct 28, 2019
View reviewed changes
Copy link
Contributor

@ia3andy ia3andy left a comment

Choose a reason for hiding this comment

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

LGTM very cool @maxandersen

@maxandersen maxandersen merged commit ac93e52 into quarkusio:master Oct 28, 2019
@gsmet gsmet added this to the 0.27.0 milestone Oct 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aloubyansky aloubyansky aloubyansky approved these changes

@ia3andy ia3andy ia3andy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.27.0
Development

Successfully merging this pull request may close these issues.
4 participants
@maxandersen @aloubyansky @ia3andy @gsmet