-
Notifications
You must be signed in to change notification settings - Fork 983
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
Fix wms maps can't load when can't request default map service #1550
Fix wms maps can't load when can't request default map service #1550
Conversation
Hey @junqiu-lei thanks for the PR! Can you add tests for these changes as the checklist item |
@tmarkley Thanks feedback, will add the test to PR. |
Are we documenting this anywhere? This seems like a breaking change. |
I have talked with @kavilla about it. Currently we have two map client setting options, OpenSearchMapsClient(extended from EMSClient) and EMSClient, we don't think the EMSClient will need to be used in any case, so that here I removed EMSClient option on the map client setting. https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/plugins/maps_legacy/public/map/service_settings.js#L134-L147 |
@junqiu-lei what was discussed? Can those details be provided here in the open? Is there any documentation that requires an update? |
Offline discussed with @tmarkley, I will update the PR to keep the EMS options there and only focus on wms maps load bug fix at no Internet condition. |
Codecov Report
@@ Coverage Diff @@
## main #1550 +/- ##
==========================================
- Coverage 68.08% 68.08% -0.01%
==========================================
Files 3072 3072
Lines 59015 59027 +12
Branches 8924 8924
==========================================
+ Hits 40183 40186 +3
- Misses 16645 16655 +10
+ Partials 2187 2186 -1
Continue to review full report at Codecov.
|
let serviceSettings; | ||
let tileMapServices; | ||
|
||
const expectedDefaultTmService = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can export and use the default setting described in opensearch_maps_client.ts
instead of defining it again here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, updated.
@@ -87,6 +100,17 @@ describe('service_settings (FKA tile_map test)', function () { | |||
return serviceSettings; | |||
} | |||
|
|||
function makeServiceSettingsWhenUnableFetchMapsService( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wow, this is a mouthful. Cant we just modify makeServiceSettings
to exit early for a failed request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, updated it into makeServiceSettings
let serviceSettings; | ||
let tileMapServices; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Cant we just define these as const
s inside the test case like the other tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, updated.
// silently ignoring the exception and returning false. | ||
return false; | ||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we changing the return value? I dont see this being used anywhere in this PR and this will default the return value of isEnabled
to true in the case of an error. Is that expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is expected. When await this._fetchWithTimeout(this._manifestServiceUrl)
fail to access our maps service(no-internet condition), it will make OpenSearchMapsClient isn't enabled, and down to use EMS client here, in this case, we can actually still use OpenSearchMapsClient rather than EMS client. And actually all our use cases can use OpenSearchMaps client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
describe('when unable to access OpenSearch maps service', function () { | ||
const expectedDefaultTmService = DEFAULT_SERVICE[0]; | ||
|
||
async function assertObject(actual, expected) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: This does not need to be an async function since you aren't awaiting anything inside it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, updated.
@@ -21,7 +21,7 @@ export class OpenSearchMapsClient extends EMSClient { | |||
result = await this._fetchWithTimeout(this._manifestServiceUrl); | |||
} catch (e) { | |||
// silently ignoring the exception and returning false. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this comment be updated since the return value is changed and we are not ignoring the exceptions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, just updated.
@@ -59,15 +60,35 @@ describe('service_settings (FKA tile_map test)', function () { | |||
}, | |||
}; | |||
|
|||
const mapConfigWithFakeManifest = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: if order is not important, maybe we could do
const mapConfigWithFakeManifest = {
...defaultMapConfig,
emsFakeManifestApiUrl,
};
to remove some duplications?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just nit stuff for comment and duplications
found fail test and updated the review
|
||
describe('service_settings (FKA tile_map test)', function () { | ||
const emsFileApiUrl = 'https://files.foobar'; | ||
const emsTileApiUrl = 'https://tiles.foobar'; | ||
const emsFakeManifestApiUrl = 'https://fakemapmanifest.foobar'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this URL is breaking the link checker. Can you either use an ignored URL from ./.lycheeexclude
or add this there. Ideally using one from there is preferred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, will update
Meanwhile, current yarn test:jest is not running this added unit test.
The reason is due to this line @junqiu-lei you can remove this line and run the test. Below is the result:
I see multiple fail tests. I think most of them are not related to your PR and we could open another issue to fix them. But I do see the following new added test fails:
I think it is fair to fix this test in the current PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unit test fail.
e9bcd57
to
b1bfb7e
Compare
@ananzh Yes, current I will update this PR's unit test to pass and then we can create a new issue to fix other tests. |
b1bfb7e
to
42f877c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the improvements!
* Fix WMS can't load when unable access maps services Signed-off-by: Junqiu Lei <junqiu@amazon.com> (cherry picked from commit b2cfb6e)
* Fix WMS can't load when unable access maps services Signed-off-by: Junqiu Lei <junqiu@amazon.com> (cherry picked from commit b2cfb6e)
* Fix WMS can't load when unable access maps services Signed-off-by: Junqiu Lei <junqiu@amazon.com> (cherry picked from commit b2cfb6e)
* Fix WMS can't load when unable access maps services Signed-off-by: Junqiu Lei <junqiu@amazon.com> (cherry picked from commit b2cfb6e)
…ct#1550) * Fix WMS can't load when unable access maps services Signed-off-by: Junqiu Lei <junqiu@amazon.com>
…ct#1550) * Fix WMS can't load when unable access maps services Signed-off-by: Junqiu Lei <junqiu@amazon.com>
…ct#1550) * Fix WMS can't load when unable access maps services Signed-off-by: Junqiu Lei <junqiu@amazon.com>
* Fix WMS can't load when unable access maps services Signed-off-by: Junqiu Lei <junqiu@amazon.com> (cherry picked from commit b2cfb6e)
* Fix WMS can't load when unable access maps services Signed-off-by: Junqiu Lei <junqiu@amazon.com> (cherry picked from commit b2cfb6e)
* Fix WMS can't load when unable access maps services Signed-off-by: Junqiu Lei <junqiu@amazon.com> (cherry picked from commit b2cfb6e)
* Fix WMS can't load when unable access maps services Signed-off-by: Junqiu Lei <junqiu@amazon.com> (cherry picked from commit b2cfb6e) Co-authored-by: Junqiu Lei <junqiu@amazon.com> Co-authored-by: Kawika Avilla <kavilla414@gmail.com>
…ct#1550) (opensearch-project#1707) * Fix WMS can't load when unable access maps services Signed-off-by: Junqiu Lei <junqiu@amazon.com> (cherry picked from commit b2cfb6e)
Signed-off-by: Junqiu Lei junqiu@amazon.com
Description
Fix the bug that users can't load their own map tiles service by custom WMS when the environment unable access to default maps service(VPC or air-gapped region).
Issues Resolved
#1506
Check List
yarn test:jest
yarn test:jest_integration
yarn test:ftr