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

Change parameter in function calls #5913

Merged
merged 7 commits into from
Nov 23, 2021
Merged

Conversation

tcnt
Copy link
Contributor

@tcnt tcnt commented Nov 5, 2021

Signed-off-by: Theo Thomas [email protected]

Description

The notebook previously called
queryKnownServers(platformName, platformServicesURLRoot)
queryActiveServers(platformName, platformServicesURLRoot)

The functions defined in common/environment-check.ipynb expects the url+portnumber which is in
platformURLroot.
Then function will internally generate platformServicesURLRoot.

Renamed platformURLroot to serverPlatformURL to align with the
name used internally in the function.
Changed function call parameter to serverPlatformURL in
place of platformServicesURLRoot

Changed function call parameter to platformURLroot in
place of platformServicesURLRoot

Fixes #5023

How Has This Been Tested?

Tested on my local kubernetes running on Raspberry PI 4's.
queryKnownServers(platformName, platformURLroot)
queryActiveServers(platformName, platformURLroot)
results in:
Querying the known servers on platform: Data Lake Platform ...
GET https://lab-datalake:9443/open-metadata/platform-services/users/garygeeke/server-platform/servers
Response:
{
"class": "ServerListResponse",
"relatedHTTPCode": 200,
"serverList": [
"governDL01",
"exchangeDL01",
"cocoMDS1",
"cocoMDS4",
"cocoOLS1",
"cocoView1"
]
}

Querying the active servers on platform: Data Lake Platform ...
GET https://lab-datalake:9443/open-metadata/platform-services/users/garygeeke/server-platform/servers/active
Response:
{
"class": "ServerListResponse",
"relatedHTTPCode": 200,
"serverList": [
"governDL01",
"exchangeDL01",
"cocoMDS1",
"cocoMDS4",
"cocoOLS1",
"cocoView1"
]
}

Any additional notes for reviewers?

The notebook called
queryKnownServers(platformName, platformServicesURLRoot)
queryActiveServers(platformName, platformServicesURLRoot)

The functions in common expects the url+portnumber which is in
platformURLroot.
Then function will internally generate platformServicesURLRoot.

Renamed platformURLroot to serverPlatformURL to align with the
name used internally in the function.
Changed function call parameter to serverPlatformURL in
place of platformServicesURLRoot

Signed-off-by: Theo Thomas <[email protected]>
@davidradl
Copy link
Member

@tcnt Please could you raise an issue associated with this pr that describes the need for the change.

@tcnt
Copy link
Contributor Author

tcnt commented Nov 17, 2021

@tcnt Please could you raise an issue associated with this pr that describes the need for the change.

As mentioned in the beginning this is for #5023 so there is already an issue for this.

@davidradl
Copy link
Member

@tcnt I am hoping to get some consensus on this one.

@mandy-chessell @planetf1 I think the existing platformURLroot is better than serverPlatformURL. Personally I would bring the internal names in line with platformURLroot . What do you think? Both words seem to be used in Egeria repo - with slightly more use of platformURLroot. In the docs repo - 20 hits on the word PlatformURLroot, 2 on serverPlatformURL.

@planetf1
Copy link
Member

On terminology generally, I think it's probably best to stick with platformURLroot, since that starting point for all our URLs is the platform URL. So you could perhaps just use platformURL instead, but then we always augment it for the particular endpoint, so root is right. Including 'server' in the name is just as technically correct as it's the root for any server requests, but I think it seems more confusing? So I would revert that rename?

@tcnt
Copy link
Contributor Author

tcnt commented Nov 21, 2021

Renaming is reverted now. After all explanations it makes sense to me.
Thanks for all the comments.

@tcnt tcnt requested a review from davidradl November 21, 2021 18:15
Copy link
Member

@planetf1 planetf1 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 fix

@mandy-chessell mandy-chessell merged commit be419e5 into odpi:master Nov 23, 2021
planetf1 pushed a commit to odpi/egeria-coco-labs that referenced this pull request Jul 25, 2022
As discussed in odpi/egeria#5913 the initial
rename of platformURLroot is reverted.

Signed-off-by: Theo Thomas <[email protected]>
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.

understanding platform services notebook fails to query servers correctly
4 participants