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.

Sign up for GitHub

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: hierarchy of variables is back #17609

Merged
merged 7 commits into from
Apr 3, 2020
Merged

fix: hierarchy of variables is back #17609

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6886a7a
fix: hierarchy of variables is back
Apr 3, 2020
f49d1b4
fix: make it optional
Apr 3, 2020
3234379
fix: omgolly jest data
Apr 3, 2020
5c92448
fix: thanks tests
Apr 3, 2020
b784576
fix: stretching out those types a bit
Apr 3, 2020
fd28666
fix: that filters itself out now
Apr 3, 2020
00aeb6a
fix: better intent
Apr 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ui/src/dashboards/actions/thunks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ export const getDashboard = (dashboardID: string) => async (
throw new Error(resp.data.message)
}

dispatch(hydrateVariables())
dispatch(hydrateVariables(true))
drdelambre marked this conversation as resolved.
Show resolved Hide resolved

const normDash = normalize<Dashboard, DashboardEntities, string>(
resp.data,
Expand Down
3 changes: 3 additions & 0 deletions ui/src/timeMachine/actions/queries.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {runStatusesQuery} from 'src/alerting/utils/statusEvents'

// Actions
import {notify} from 'src/shared/actions/notifications'
import {hydrateVariables} from 'src/variables/actions/thunks'

// Constants
import {rateLimitReached, resultTooLarge} from 'src/shared/copy/notifications'
Expand Down Expand Up @@ -115,6 +116,8 @@ export const executeQueries = () => async (dispatch, getState: GetState) => {
try {
dispatch(setQueryResults(RemoteDataState.Loading, [], null))

await dispatch(hydrateVariables())
Copy link
Contributor Author

@drdelambre drdelambre Apr 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

this refreshes variables that haven't been fetched by another means before sending off the query, just in case. found this while deep linking w/ dependent variables and no application state.

uses built in cache mechanism to ensure we're not over fetching


//TODO: replace with activeContext selector
const contextID =
activeTimeMachine.contextID || state.timeMachines.activeTimeMachineID
Expand Down
4 changes: 2 additions & 2 deletions ui/src/variables/actions/thunks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ export const getVariables = () => async (
}
}

export const hydrateVariables = () => async (
export const hydrateVariables = (skipCache?: boolean) => async (
dispatch: Dispatch<Action>,
getState: GetState
) => {
Expand All @@ -107,7 +107,7 @@ export const hydrateVariables = () => async (
const vals = await hydrateVars(vars, getAllVariablesFromState(state), {
orgID: org.id,
url: state.links.query.self,
skipCache: true,
skipCache,
}).promise

vars
Expand Down
8 changes: 8 additions & 0 deletions ui/src/variables/selectors/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,11 @@ export const getVariable = (
if (!vari.selected) {
if (vari.arguments.type === 'map') {
vari.selected = [Object.keys(vari.arguments.values)[0]]
} else if (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unrelated change, just an oversight I caught while debugging

vari.arguments.type === 'query' &&
vari.arguments.values.results
) {
vari.selected = [vari.arguments.values.results[0]]
} else {
vari.selected = [vari.arguments.values[0]]
}
Expand Down Expand Up @@ -252,6 +257,9 @@ export const asAssignment = (variable: Variable): VariableAssignment => {
}

if (variable.arguments.type === 'query') {
if (!variable.selected[0]) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the ensures that the AST doesn't return variables if they don't have a value set, because that causes all sorts of issues in the LSP and on the api

return null
}
out.init = {
type: 'StringLiteral',
value: variable.selected[0],
Expand Down
2 changes: 1 addition & 1 deletion ui/src/variables/utils/buildVarsOption.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export const buildVarsOption = (variables: VariableAssignment[]): File => ({
},
init: {
type: 'ObjectExpression',
properties: variables.map(assignmentToProperty),
properties: variables.filter(v => !!v).map(assignmentToProperty),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

and this makes sure those variables we removed are filtered from the list before further processing

},
},
},
Expand Down
3 changes: 3 additions & 0 deletions ui/src/variables/utils/hydrateVars.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,10 @@ export const hydrateVars = (
node.status === RemoteDataState.Loading

try {
// TODO: remove the concept of node.values, just use node.variable
node.values = await hydrateVarsHelper(node, options)
node.variable.arguments.values.results = node.values.values
Copy link
Contributor Author

@drdelambre drdelambre Apr 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

here is the actual fix for this PR. there's more to be done to further enforce the variable data structure, as mentioned in the TODO

node.variable.selected = node.values.selected
node.status = RemoteDataState.Done

return Promise.all(node.parents.filter(readyToResolve).map(resolve))
Expand Down