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

Templating forward port for github codespace #1505

Merged
merged 1 commit into from May 2, 2022
Merged

Templating forward port for github codespace #1505

merged 1 commit into from May 2, 2022

Conversation

matthieuRioual
Copy link
Contributor

I replaced the forward port file with mustache extension to allow import port when importing the file.

@codecov
Copy link

codecov bot commented Apr 27, 2022 •
edited

Codecov Report

Merging #1505 (ab77147) into main (c2441da) will not change coverage.
The diff coverage is 100.00%.

❗ Current head ab77147 differs from pull request most recent head 5e112ff. Consider uploading reports for the commit 5e112ff to get more accurate results

@@             Coverage Diff              @@
##                main     #1505    +/-   ##
============================================
  Coverage     100.00%   100.00%            
+ Complexity      1612      1609     -3     
============================================
  Files            319       331    +12     
  Lines           5354      5468   +114     
  Branches         107       110     +3     
============================================
+ Hits            5354      5468   +114
Impacted Files Coverage Δ
...tup/codespaces/domain/CodespacesDomainService.java 100.00% <100.00%> (ø)
src/main/webapp/app/common/domain/Service.ts 100.00% <0.00%> (ø)
.../main/webapp/app/common/secondary/RestServiceId.ts 100.00% <0.00%> (ø)
...bapp/app/springboot/primary/Generator.component.ts 100.00% <0.00%> (ø)
...bapp/app/springboot/secondary/ProjectRepository.ts 100.00% <0.00%> (ø)
...p/app/common/secondary/ProjectHistoryRepository.ts 100.00% <0.00%> (ø)
...p/app/springboot/secondary/SpringBootRepository.ts 100.00% <0.00%> (ø)
...p/app/springboot/secondary/client/VueRepository.ts 100.00% <0.00%> (ø)
...app/springboot/secondary/client/ReactRepository.ts 100.00% <0.00%> (ø)
...p/springboot/secondary/client/AngularRepository.ts 100.00% <0.00%> (ø)
... and 38 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2441da...5e112ff. Read the comment docs.

pascalgrimaud
pascalgrimaud requested changes Apr 27, 2022
View reviewed changes
src/main/java/tech/jhipster/lite/generator/setup/codespaces/domain/CodespacesDomainService.java Outdated
@@ -21,7 +21,8 @@ public void init(Project project) {
}

private void addConfig(Project project) {
projectRepository.add(project, SOURCE, "devcontainer.json", DEVCONTAINER_DEST, "devcontainer.json");
project.addConfig("serverPort", 7471);
Copy link
Member

Choose a reason for hiding this comment

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

the default port of all Spring Boot application is 8080
7471 is only related to JHipster Lite

src/main/resources/generator/setup/codespaces/devcontainer.json.mustache Outdated
@@ -23,7 +23,7 @@
"christian-kohler.npm-intellisense"
],

"forwardPorts": [3001, 4200, 8080, 9000, 18080],
"forwardPorts": {{serverPort}},
Copy link
Member

Choose a reason for hiding this comment

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

you need to keep 9000 too, as 9000 is used when you launch the front with npm start

Copy link
Member

Choose a reason for hiding this comment

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

This file needs to be renamed with mustache template too

Copy link
Member

Choose a reason for hiding this comment

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

@matthieulapatate : your initial change was good, we should only have here: {{serverPort}} and 9000
3001, 4200, 18080 need to be removed

@pascalgrimaud pascalgrimaud added area: bug 🐛 Something isn't working theme: init labels Apr 27, 2022
pascalgrimaud
pascalgrimaud requested changes Apr 28, 2022
View reviewed changes
Copy link
Member

@pascalgrimaud pascalgrimaud left a comment

Choose a reason for hiding this comment

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

Waiting your fix after my review

src/main/resources/generator/setup/codespaces/devcontainer.json.mustache Outdated
@@ -23,7 +23,7 @@
"christian-kohler.npm-intellisense"
],

"forwardPorts": [3001, 4200, 8080, 9000, 18080],
"forwardPorts": {{serverPort}},
Copy link
Member

Choose a reason for hiding this comment

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

This file needs to be renamed with mustache template too

pascalgrimaud
pascalgrimaud requested changes Apr 29, 2022
View reviewed changes
Copy link
Member

@pascalgrimaud pascalgrimaud left a comment

Choose a reason for hiding this comment

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

the Dockerfile doesn't contain any variable, so no need to be mustache

Only devcontainers.json has variable, so this one need to be mustache

pascalgrimaud
pascalgrimaud requested changes Apr 29, 2022
View reviewed changes
Copy link
Member

@pascalgrimaud pascalgrimaud left a comment

Choose a reason for hiding this comment

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

plz, squash all your commits at the end too

src/main/resources/generator/setup/codespaces/devcontainer.json.mustache Outdated Show resolved Hide resolved
@pascalgrimaud
Copy link
Member

Still fail. You need to launch local unit tests each time you modify code, it's faster than pushing and waiting the result from CI

@pascalgrimaud pascalgrimaud marked this pull request as draft April 30, 2022 15:55
@pascalgrimaud
Copy link
Member

I convert it to draft so:

  • once the tests are fixed
  • once the commits are squash
  • change it to ready for review

@matthieuRioual matthieuRioual marked this pull request as ready for review May 2, 2022 01:11
@pascalgrimaud pascalgrimaud merged commit 69ebb7e into jhipster:main May 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pascalgrimaud pascalgrimaud pascalgrimaud approved these changes

Assignees
No one assigned
Labels
area: bug 🐛 Something isn't working theme: init
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Codespaces: forwardPorts and serverPort
2 participants
@matthieuRioual @pascalgrimaud