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(ui): resolved telegraf bucket dropdown bug and undefined token issue #17363

Merged
merged 4 commits into from
Mar 20, 2020

Conversation

asalem1
Copy link
Contributor

@asalem1 asalem1 commented Mar 19, 2020

Closes #17118

Problem

  1. The default buckets that were being passed were passing the system buckets in when they should've been filtering them out
  2. Fetching tokens were broken in the normalization process due to a tenuous connection that existed between Telegraf Configs, Telegrafs, Labels, and Tokens

Solution

  1. Passed in the correct selector to filter out the buckets to remove system buckets from the dropdown
  2. After speaking with @russorat and @121watts, it became clear that tokens should be more in line with other conventions that are currently used by Apple, Github, etc... in that tokens that are generated should never be retrieved. That is to say that a token that is created should be stored by the user. If they lose their token, we should give them an opportunity to generate a new token.

After speaking with @alexpaxton, it became clear that having a button underneath the code snippet made more sense to include rather than having a small button on the same line as the code

telegraf-token-fix

  • CHANGELOG.md updated with a link to the PR (not the Issue)
  • Rebased/mergeable

@asalem1 asalem1 requested a review from 121watts March 19, 2020 22:19
@asalem1 asalem1 force-pushed the fix/telegraf-buckets-bug branch from f7337a6 to 45b2e34 Compare March 19, 2020 22:23
try {
const state = getState()
const org = getOrg(state)
const telegraf = get(state, `resources.telegrafs.byID.${configID}`, '')
Copy link
Contributor

Choose a reason for hiding this comment

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

already a selector for this getByID<Telegraf>(state, id)

const org = getOrg(state)
const telegraf = get(state, `resources.telegrafs.byID.${configID}`, '')
const bucketName = get(telegraf, 'metadata.buckets[0]', '')
const bucket = getBucketByName(state, bucketName)
Copy link
Contributor

Choose a reason for hiding this comment

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

should handle no bucket name being on the telegraf config.

import {isSystemBucket} from 'src/buckets/selectors'

// Utils
import {isSystemBucket} from 'src/buckets/constants/index'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import {isSystemBucket} from 'src/buckets/constants/index'
import {isSystemBucket} from 'src/buckets/constants'

state: AppState,
bucketName: string
): Bucket => {
const buckets = state.resources.buckets.byID
Copy link
Contributor

Choose a reason for hiding this comment

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

there's already a selector for this also which will get all buckets and return them in a list for you 😄

}

return auth.token
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

const config = {
...tc,
labels: [label],
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

@asalem1 asalem1 merged commit 42ad381 into master Mar 20, 2020
@asalem1 asalem1 deleted the fix/telegraf-buckets-bug branch March 20, 2020 03:01
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.

Telegraf Config Defaults to System Bucket
2 participants