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

docs: change port for KTL docs #713

Merged
merged 3 commits into from
Feb 1, 2023

Conversation

sudiptob2
Copy link
Member

@sudiptob2 sudiptob2 commented Jan 28, 2023

KTL docs port changes so that id does not conflict with the keptn docs (#708)

  • default external docker port for hugo changed to 1314
  • Internal hugo port changed to 1314 so that the user gets the correct msg about where the server is running
  • hugo port made configurable using PORT variable

image

Example:
make PORT=1319 server

@sudiptob2 sudiptob2 changed the title different port for KTL docs docs:different port for KTL docs Jan 28, 2023
@sudiptob2 sudiptob2 force-pushed the docs/708/different-port-for-ktl-docs branch from 3f3d005 to 4037aa2 Compare January 28, 2023 04:55
@sudiptob2 sudiptob2 changed the title docs:different port for KTL docs docs: different port for KTL docs Jan 28, 2023
@aepfli
Copy link
Member

aepfli commented Jan 28, 2023

Yes, this is correct, but it renders the user with another problem if they use the makefile to start the server - because Hugo thinks it still runs on port 1313 within the docker container and prints this information to the user.

image

We would also need to adapt the port for Hugo, so Hugo itself within the container is starting on the same port. Otherwise, the user experience will suffer, and finding out which port to use will be hard.

As you seem like a curious person, I won't immediately post the solution :) Sometimes it is fun to figure things out on your own. But I can provide you with the necessary steps if needed.

@sudiptob2
Copy link
Member Author

@aepfli thanks for giving the hint. I will try to investigate myself and hope to find the solution ✔️

@sudiptob2 sudiptob2 force-pushed the docs/708/different-port-for-ktl-docs branch from 57aad32 to acbbfb9 Compare January 28, 2023 09:49
@sudiptob2
Copy link
Member Author

@aepfli , I just updated the internal port of Hugo. Could you comment on the new changes, please.

aepfli
aepfli previously approved these changes Jan 28, 2023
Copy link
Member

@aepfli aepfli left a comment

Choose a reason for hiding this comment

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

perfect - thank you for the contribution

the only thing, we could theoretically improve is to extract this port number to a variable, so it is easier to change :)

@sudiptob2
Copy link
Member Author

Thanks for reviewing and approving the PR

the only thing, we could theoretically improve is to extract this port number to a variable, so it is easier to change :)

I will update the code to accommodate this improvement also.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sudiptob2 sudiptob2 requested a review from aepfli January 28, 2023 14:09
Copy link
Contributor

@thschue thschue left a comment

Choose a reason for hiding this comment

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

lgtm

@sudiptob2
Copy link
Member Author

Hi, @aepfli thanks for approving the PR. This PR seems ready to be merged. Please let me know if I have to do anything from my end.
I appreciate your time and effort reviewing this PR

@aepfli
Copy link
Member

aepfli commented Feb 1, 2023

I sadly do not have more powers than you have within this repository, maybe @thisthat @StackScribe or @StackScribe can help?

@codecov
Copy link

codecov bot commented Feb 1, 2023

Codecov Report

Merging #713 (20964c4) into main (a9f41d7) will increase coverage by 0.02%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #713      +/-   ##
==========================================
+ Coverage   57.33%   57.36%   +0.02%     
==========================================
  Files          88       88              
  Lines        7001     7001              
==========================================
+ Hits         4014     4016       +2     
+ Misses       2821     2820       -1     
+ Partials      166      165       -1     
Impacted Files Coverage Δ
...lers/lifecycle/keptnworkloadinstance/controller.go 80.99% <0.00%> (-0.46%) ⬇️
...ptnworkloadinstance/reconcile_prepostdeployment.go 90.90% <0.00%> (+9.09%) ⬆️
Flag Coverage Δ
component-tests 53.33% <ø> (-0.18%) ⬇️
keptn-cert-manager 67.50% <ø> (ø)
keptn-lifecycle-operator 53.05% <ø> (ø)
scheduler 21.17% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@thisthat thisthat changed the title docs: different port for KTL docs docs: change port for KTL docs Feb 1, 2023
@thisthat thisthat merged commit 517e148 into keptn:main Feb 1, 2023
@keptn-bot keptn-bot mentioned this pull request Feb 1, 2023
@sudiptob2 sudiptob2 deleted the docs/708/different-port-for-ktl-docs branch February 7, 2023 16:22
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.

4 participants