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

feat(pkger): add ability to export resources by name from cli #19457

Merged
merged 22 commits into from
Sep 23, 2020
Merged

Conversation

glinton
Copy link
Contributor

@glinton glinton commented Aug 27, 2020

Closes #18733

Export resources by name via the cli.

@glinton glinton changed the title Feat/18733 feat(pkger): add ability to export resources by name from cli Aug 27, 2020
@glinton glinton marked this pull request as ready for review September 11, 2020 22:47
@glinton glinton requested a review from a team as a code owner September 11, 2020 22:47
@glinton glinton requested review from a team and hoorayimhelping and removed request for a team September 11, 2020 22:51
Copy link
Contributor

@hoorayimhelping hoorayimhelping left a comment

Choose a reason for hiding this comment

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

No hard blockers, just a couple of questions

cmd/influxd/launcher/pkger_test.go Outdated Show resolved Hide resolved
@@ -27,7 +27,7 @@ type NameGenerator func() string
// ResourceToClone is a resource that will be cloned.
type ResourceToClone struct {
Kind Kind `json:"kind"`
ID influxdb.ID `json:"id"`
ID influxdb.ID `json:"id,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

do you know if any frontend code depends on this? changing how it handles empty might require changes on the javascript side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'm not certain the relationship, but it is required to be omitted when exporting by name (specifying only name in the payload). i suppose the swagger should be updated to note that it is only required when no name is specified and optional if it is specified. ...i don't love how if both are specified, the resource is found by the id and the exported result is renamed to the name provided.

pkger/clone_resource.go Outdated Show resolved Hide resolved
pkger/clone_resource.go Outdated Show resolved Hide resolved
pkger/service.go Outdated Show resolved Hide resolved
pkger/service.go Outdated Show resolved Hide resolved
@@ -148,7 +148,8 @@ func (s *loggingMW) Export(ctx context.Context, opts ...ExportOptFn) (template *
s.logger.Error("failed to export template", zap.Error(err), dur)
return
}
s.logger.Info("failed to export template", append(s.summaryLogFields(template.Summary()), dur)...)
// todo: should these be Debug logs?
Copy link
Contributor

Choose a reason for hiding this comment

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

@russorat are you the man to answer this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on what i see in other parts of the app, i would think these "failed to" logs should be debug.

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 failed to seemed to be copypasta. the logs in question are the success logs

https://github.com/influxdata/influxdb/pull/19457/files/53b325c7d0d16ce11723083cd3a0243e481881ce#diff-11efed3cf4ce3c7284b325143fb8b70dR180-R181

s.logger.Info("template dry run successful", fields...)
// ...
s.logger.Info("template apply successful", fields...)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, if they are success logs, i would think they should be Info

@glinton glinton requested a review from a team as a code owner September 21, 2020 20:24
Copy link
Contributor

@hoorayimhelping hoorayimhelping left a comment

Choose a reason for hiding this comment

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

Works for me, but I haven't tested the swagger definition with yarn generate. is the swagger def good to go?

pkger/service_logging.go Outdated Show resolved Hide resolved
@@ -135,7 +135,9 @@ func filterDashboardsFn(filter influxdb.DashboardFilter) func(d *influxdb.Dashbo
}
}

return func(d *influxdb.Dashboard) bool { return true }
return func(d *influxdb.Dashboard) bool {
return ((filter.OrganizationID == nil) || (*filter.OrganizationID == d.OrganizationID))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hoping this is kosher, it follows the pattern of labels

func filterLabelsFn(filter influxdb.LabelFilter) func(l *influxdb.Label) bool {
	return func(label *influxdb.Label) bool {
		return (filter.Name == "" || (strings.EqualFold(filter.Name, label.Name))) &&
			((filter.OrgID == nil) || (filter.OrgID != nil && *filter.OrgID == label.OrgID))
	}
}

@glinton glinton merged commit 85d75e3 into master Sep 23, 2020
@glinton glinton deleted the feat/18733 branch September 23, 2020 17: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.

Provide ability to export by name for all resources
3 participants