-
Notifications
You must be signed in to change notification settings - Fork 18
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
ENG-2964 GetEnvironment RTKQuery #1343
ENG-2964 GetEnvironment RTKQuery #1343
Conversation
@@ -73,6 +74,5 @@ const ( | |||
GetWorkflowDagResultRoute = "/api/workflow/{workflowId}/result/{workflowDagResultId}" | |||
GetWorkflowHistoryRoute = "/api/workflow/{workflowId}/history" | |||
|
|||
GetServerVersionRoute = "/api/version" |
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.
Is any SDK code still using this route? To be safe, we could keep these routes and map them to the same handler (similar to workflow trigger / delete / edit routes)
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, the version route does have a usage. I will keep this one in place.
@@ -123,22 +123,19 @@ const MenuSidebar: React.FC<{ | |||
const [versionNumber, setVersionNumber] = useState(''); | |||
const location = useLocation(); | |||
|
|||
const { data } = useEnvironmentGetQuery({ apiKey: user.apiKey } as any, { |
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.
no need to cast to any
and you probably don't need the skip
here, as the user is passed by caller
const versionNumberResponse = await res.json(); | ||
setVersionNumber(versionNumberResponse.version); | ||
if (data) { | ||
setVersionNumber(data.version); |
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.
will the user or other things set the version number? Otherwise we can just do const versionNumber = data?.version
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.
Nope, I will remove this useEffect and state variable.
Vestiges here from the infinite loop that I was facing with the Kubernetes dialog :)
Describe your changes and why you are making these changes
This PR moves the getEnvironment API call from the kubernetes dialog and the menusidebar to use RTKQuery.
Also moves the getEnvironment route to the new v2 routes.
Related issue number (if any)
ENG-2964
Loom demo (if any)
Checklist before requesting a review
python3 scripts/run_linters.py -h
for usage).run_integration_test
: Runs integration testsskip_integration_test
: Skips integration tests (Should be used when changes are ONLY documentation/UI)