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

Added missing JDL deployment options #14945

Merged
merged 8 commits into from
Jun 9, 2021
Merged

Conversation

MathieuAA
Copy link
Member

@MathieuAA MathieuAA commented May 11, 2021

This PR adds and fixes some JDL deployment options as reported by @mraible on #14817.
This PR is still a draft, so expect some changes.


Please make sure the below checklist is followed for Pull Requests.

When you are still working on the PR, consider converting it to Draft (bellow reviewers) and adding skip-ci label, you can still see CI build result at your branch.

Fixes #14817

@MathieuAA MathieuAA self-assigned this May 11, 2021
@MathieuAA
Copy link
Member Author

@mraible this PR is now in a working state, but I'm not comfortable with making "ready for review" just yet.
I'm gonna spend more time reviewing the options because I'm pretty sure there are some things related to deployment options that I haven't updated yet.

@MathieuAA
Copy link
Member Author

MathieuAA commented May 14, 2021

Some news: I'm done adding missing deployment options. However, validations are missing so I have to add them.

@MathieuAA MathieuAA marked this pull request as ready for review June 1, 2021 18:56
@MathieuAA MathieuAA requested a review from mraible June 1, 2021 18:56
Copy link
Member

@deepu105 deepu105 left a comment

Choose a reason for hiding this comment

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

Looks good. But I havent tested it. I'm trusting the tests here

storageType: {
ephemeral: 'ephemeral',
persistent: 'persistent',
},
};

Options.defaults = (deploymentType = Options.deploymentType.dockerCompose) =>
_.omitBy(
Copy link
Member

Choose a reason for hiding this comment

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

I dont remmber why did omitBy here originally, so hope its still works

Copy link
Member Author

Choose a reason for hiding this comment

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

The omit here is useful if you have undefined values, and as the previous code generated them, the omit's role was to clean things up.

Copy link
Member Author

Choose a reason for hiding this comment

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

And thanks for the review :)

@MathieuAA
Copy link
Member Author

Thanks @DanielFran for the labels! @mraible do you have time to checkout this branch and test it out?

@mraible
Copy link
Contributor

mraible commented Jun 7, 2021

I tried this branch with the following JDL:

deployment {
  deploymentType kubernetes
  appsFolders [gateway, blog, store]
  clusteredDbApps [store]
  dockerRepositoryName "gcr.io/jhipster7"
  kubernetesNamespace demo
  kubernetesServiceType LoadBalancer
  kubernetesUseDynamicStorage true
}

The resulting .yo-rc.json has almost everything. However, it's missing:

"kubernetesStorageClassName": ""

If I try to add this to my JDL:

deployment {
  deploymentType kubernetes
  appsFolders [gateway, blog, store]
  clusteredDbApps [store]
  dockerRepositoryName "gcr.io/jhipster7"
  kubernetesNamespace demo
  kubernetesServiceType LoadBalancer
  kubernetesUseDynamicStorage true
  kubernetesStorageClassName ""
}

I get an error.

error: Syntax error message:
	A name is expected, but found: """"
	at line: 9, column: 30
The kubernetesStorageClassName property name must match: /^[A-Za-z]+$/, got "".
	at line: 9, column: 30
Error during import-jdl: Error: A name is expected, but found: """"
	at line: 9, column: 30
The kubernetesStorageClassName property name must match: /^[A-Za-z]+$/, got "".
	at line: 9, column: 30

If I use the value that the prompts give me (empty parenthesis instead of blank):

deployment {
  deploymentType kubernetes
  appsFolders [gateway, blog, store]
  clusteredDbApps [store]
  dockerRepositoryName "gcr.io/jhipster7"
  kubernetesNamespace demo
  kubernetesServiceType LoadBalancer
  kubernetesUseDynamicStorage true
  kubernetesStorageClassName ()
}

It fails too:

ERROR! Error during import-jdl: NoViableAltException: Expecting: one of these possible Token sequences:
  1. [BOOLEAN]
  2. [NAME, '.']
  3. [NAME]
  4. ['[']
  5. [INTEGER]
  6. [STRING]
but found: '('
	at line: 9, column: 30

@MathieuAA
Copy link
Member Author

Alright, thanks @mraible. I'm fixing it.

@MathieuAA
Copy link
Member Author

Okay, the fixed is pushed.
The documentation should be updated for the issue to be truly "closed".

@mraible
Copy link
Contributor

mraible commented Jun 8, 2021

LGTM!

@MathieuAA
Copy link
Member Author

@mraible if it's okay with you, I wanna create a PR to update the docs before merging it.

@MathieuAA MathieuAA merged commit 4bffad3 into jhipster:main Jun 9, 2021
@MathieuAA MathieuAA deleted the fix14817 branch June 9, 2021 06:05
@pascalgrimaud pascalgrimaud added this to the 7.1.0 milestone Jun 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deployment JDL doesn't offer all Kubernetes options
5 participants