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

add optional api-server prefix #1126

Merged
merged 4 commits into from
Feb 4, 2021
Merged

add optional api-server prefix #1126

merged 4 commits into from
Feb 4, 2021

Conversation

yehiyam
Copy link
Contributor

@yehiyam yehiyam commented Feb 1, 2021

This change is Reviewable

Copy link
Contributor

@nassiharel nassiharel left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r1.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @yehiyam)


core/api-server/config/main/config.base.js, line 56 at r1 (raw file):

 path: process.env.BASE_URL_PATH? path.join(config.ingressPrefix, process.env.BASE_URL_PATH): config.ingressPrefix

Writing this is the same?
path: path.join(config.ingressPrefix, process.env.BASE_URL_PATH || '')

Copy link
Contributor Author

@yehiyam yehiyam left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @nassiharel)


core/api-server/config/main/config.base.js, line 56 at r1 (raw file):

Previously, NassiHarel (Nassi Harel) wrote…
 path: process.env.BASE_URL_PATH? path.join(config.ingressPrefix, process.env.BASE_URL_PATH): config.ingressPrefix

Writing this is the same?
path: path.join(config.ingressPrefix, process.env.BASE_URL_PATH || '')

if both ingressPath and BASE_URL are empty your option returns . and not empty string. Added tests to verify

@yehiyam yehiyam requested a review from nassiharel February 3, 2021 07:38
Copy link
Contributor

@nassiharel nassiharel left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 2 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yehiyam)


core/api-server/config/main/config.base.js, line 56 at r1 (raw file):

 path.join

Yes
"If the joined path string is a zero-length string then '.' will be returned, representing the current working directory."

https://nodejs.org/api/path.html#path_path_join_paths

Copy link
Contributor Author

@yehiyam yehiyam left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nassiharel)


core/api-server/config/main/config.base.js, line 56 at r1 (raw file):

Previously, NassiHarel (Nassi Harel) wrote…
 path.join

Yes
"If the joined path string is a zero-length string then '.' will be returned, representing the current working directory."

https://nodejs.org/api/path.html#path_path_join_paths

Done.

@yehiyam yehiyam enabled auto-merge (squash) February 4, 2021 13:24
@yehiyam yehiyam merged commit c9df25b into master Feb 4, 2021
@yehiyam yehiyam deleted the fix-swagger-server-prefix branch February 4, 2021 13:27
hkube-ci pushed a commit that referenced this pull request Feb 4, 2021
* add optional api-server prefix

* add tests .... bump version [skip ci]
hkube-ci pushed a commit that referenced this pull request Feb 4, 2021
* add optional api-server prefix

* add tests .... bump version [skip ci]
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.

2 participants