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

[Fix] ValidationError when setting maps.proxyOpenSearchMapsServiceInMaps #5170

Merged
merged 5 commits into from
Oct 18, 2023

Conversation

ShatilKhan
Copy link
Contributor

Description

Swapped the arguments that has been causing Validations error.
swapped places of map.proxyElasticMapsServiceInMaps with map.proxyOpenSearchMapsServiceInMaps

Issues Resolved

Fixes #5122

Screenshot

Testing the changes

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
    • yarn test:ftr
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

Signed-off-by: ShatilKhan <shatilshahriar009@gmail.com>
Signed-off-by: ShatilKhan <shatilshahriar009@gmail.com>
@joshuarrrr
Copy link
Member

@ShatilKhan Thanks for taking this on! One thing we want from all PRs is a summary of how to test and verify that the fix in the PR actually solves the problem in the issue. Oftentimes that means writing additional tests or test cases. But sometimes it can just be a set of steps, such as in this PR description: #5168

Screenshots or videos can also be helpful in demonstrating the fix, particularly for UI changes.

In this case, you probably want to do 2 things:

  1. Make sure you can reproduce the problem as described in [BUG] ValidationError when setting maps.proxyOpenSearchMapsServiceInMaps  #5122. If you can't reproduce on main, there's no way to know if your changes actually fix the issue.
  2. On this branch, go through the same reproductions steps to demonstrate that the problem is resolved.

A description of this testing can be added to the "Testing the changes" section in the description. That way any reviewer also can go through the steps if needed.

CHANGELOG.md Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 2, 2023

Codecov Report

Merging #5170 (2d33449) into main (60f4c8f) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #5170   +/-   ##
=======================================
  Coverage   66.77%   66.77%           
=======================================
  Files        3284     3284           
  Lines       63095    63095           
  Branches    10049    10049           
=======================================
  Hits        42134    42134           
  Misses      18488    18488           
  Partials     2473     2473           
Flag Coverage Δ
Linux_1 35.26% <ø> (ø)
Linux_2 55.24% <ø> (ø)
Linux_3 43.85% <ø> (ø)
Linux_4 35.36% <ø> (ø)
Windows_1 35.27% <ø> (ø)
Windows_2 55.20% <ø> (ø)
Windows_3 43.85% <ø> (ø)
Windows_4 35.36% <ø> (ø)

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

AMoo-Miki and others added 2 commits October 3, 2023 19:38
Signed-off-by: Miki <amoo_miki@yahoo.com>
Signed-off-by: Miki <miki@amazon.com>
@AMoo-Miki
Copy link
Collaborator

I manually made the change suggested by @joshuarrrr and resolved the conflict, in the interest of speeding up the process.

@AMoo-Miki AMoo-Miki requested a review from joshuarrrr October 4, 2023 02:41
@ShatilKhan
Copy link
Contributor Author

Thanks for fixing the CHANGELOG @AMoo-Miki
I was a bit busy, I am working on reproducing the bug and adding a few screenshots. I'm not sure if someone else's change will be accepted in this PR. But thanks anyway

@tobiasehlert
Copy link
Contributor

Thanks for fixing the CHANGELOG @AMoo-Miki I was a bit busy, I am working on reproducing the bug and adding a few screenshots. I'm not sure if someone else's change will be accepted in this PR. But thanks anyway

You see in my original issue (#5122 (comment)) what the config was that caused the error to appear.

If you don't get the ValidationError message when configuring the map.proxyOpenSearchMapsServiceInMaps value, then your suggested change should be successful, if not then you are not done yet.

I could have tried writing this PR too, but you wanted to so I'll wait ^_^

@ShatilKhan
Copy link
Contributor Author

hey @tobiasehlert
Thank you so much for the opportunity. I was a bit busy & still trying to figure things out with this codebase. I'll make an update soon.

@tobiasehlert
Copy link
Contributor

@ShatilKhan, how is it going for you? :)

@ShatilKhan
Copy link
Contributor Author

Yeah I'm on it @tobiasehlert
I'll give update by tomorrow

@joshuarrrr joshuarrrr added the OSCI Open Source Contributor Initiative label Oct 11, 2023
Signed-off-by: Josh Romero <rmerqg@amazon.com>
@ShatilKhan
Copy link
Contributor Author

Hi @joshuarrrr
Apologies for my delay
Has this PR been merged?
Should I reproduce the error & send screenshots?

@tobiasehlert
Copy link
Contributor

@ShatilKhan, no the PR is still open so your suggested fix is not implemented yet and waiting for you :)

@joshuarrrr
Copy link
Member

@ShatilKhan Reviewers were awaiting some verification testing. Here's an example of how I verified this PR:

  1. Reproductions of issue as described in [BUG] ValidationError when setting maps.proxyOpenSearchMapsServiceInMaps  #5122
Screen.Recording.2023-10-16.at.5.21.10.PM.mov
  1. Demonstration of the fix (with this PR branch):
Screen.Recording.2023-10-16.at.5.25.18.PM.mov

Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

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

LGTM

@ananzh ananzh merged commit 466d298 into opensearch-project:main Oct 18, 2023
69 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 18, 2023
…Maps` (#5170)

* Update index.ts args

---------

Signed-off-by: ShatilKhan <shatilshahriar009@gmail.com>
Signed-off-by: Miki <amoo_miki@yahoo.com>
Signed-off-by: Miki <miki@amazon.com>
Signed-off-by: Josh Romero <rmerqg@amazon.com>
Co-authored-by: Miki <amoo_miki@yahoo.com>
Co-authored-by: Miki <miki@amazon.com>
Co-authored-by: Josh Romero <rmerqg@amazon.com>
(cherry picked from commit 466d298)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
@ShatilKhan
Copy link
Contributor Author

Thanks @joshuarrrr
I'll make sure to give regular updates on my upcoming PRs
I was a bit confused about reproducing the issue on this one
Thanks for explaining that part as well!

manasvinibs pushed a commit that referenced this pull request Oct 26, 2023
…Maps` (#5170)

* Update index.ts args

---------

Signed-off-by: ShatilKhan <shatilshahriar009@gmail.com>
Signed-off-by: Miki <amoo_miki@yahoo.com>
Signed-off-by: Miki <miki@amazon.com>
Signed-off-by: Josh Romero <rmerqg@amazon.com>
Co-authored-by: Miki <amoo_miki@yahoo.com>
Co-authored-by: Miki <miki@amazon.com>
Co-authored-by: Josh Romero <rmerqg@amazon.com>
(cherry picked from commit 466d298)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
ananzh pushed a commit that referenced this pull request Nov 10, 2023
…Maps` (#5170) (#5318)

* Update index.ts args

---------

Signed-off-by: ShatilKhan <shatilshahriar009@gmail.com>
Signed-off-by: Miki <amoo_miki@yahoo.com>
Signed-off-by: Miki <miki@amazon.com>
Signed-off-by: Josh Romero <rmerqg@amazon.com>
Co-authored-by: Miki <amoo_miki@yahoo.com>
Co-authored-by: Miki <miki@amazon.com>
Co-authored-by: Josh Romero <rmerqg@amazon.com>
(cherry picked from commit 466d298)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] ValidationError when setting maps.proxyOpenSearchMapsServiceInMaps
5 participants