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

Add event publishing to kne_cli and controller server #393

Merged
merged 18 commits into from
Jul 13, 2023
Merged

Add event publishing to kne_cli and controller server #393

merged 18 commits into from
Jul 13, 2023

Conversation

alexmasi
Copy link
Contributor

No description provided.

@github-actions
Copy link

github-actions bot commented Jun 30, 2023

Pull Request Test Coverage Report for Build 5471004216

  • 11 of 102 (10.78%) changed or added relevant lines in 5 files are covered.
  • 25 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-1.1%) to 65.012%

Changes Missing Coverage Covered Lines Changed/Added Lines %
cmd/topology/topology.go 7 9 77.78%
cmd/deploy/deploy.go 2 5 40.0%
topo/topo.go 2 10 20.0%
deploy/deploy.go 0 30 0.0%
cmd/root.go 0 48 0.0%
Files with Coverage Reduction New Missed Lines %
cmd/root.go 25 5.44%
Totals Coverage Status
Change from base Build 5418025002: -1.1%
Covered Lines: 3666
Relevant Lines: 5639

💛 - Coveralls

@alexmasi
Copy link
Contributor Author

alexmasi commented Jul 6, 2023

Now all of the functionality is in place, cmd line flags can now be supplied with a config file using viper:

$ cat ~/.kne/config.yaml
report_usage: false
progress: true

This example sets the default values of the --report_usage and --progress flags. The hierarchy for flag values is as follows:

  1. The value specified as a flag when calling the CLI
  2. The value found in the kne config file
  3. The default value specified using viper.SetDefault()
  4. The default value specified using the flag default

@alexmasi
Copy link
Contributor Author

alexmasi commented Jul 6, 2023

Next step is to break into 3 PRs:

  1. Adding viper and restructure CLI
  2. Adding usage pkg
  3. Adding event publishing to deploy.go, topo.go, and the necessary flags to set them from the CLI and controller server

@alexmasi alexmasi changed the title Add event publishing + kne config file Add event publishing to kne_cli and controller server Jul 7, 2023
@alexmasi
Copy link
Contributor Author

alexmasi commented Jul 7, 2023

need to merge from main after #396 is merged

@alexmasi
Copy link
Contributor Author

alexmasi commented Jul 7, 2023

/gcbrun

@alexmasi alexmasi marked this pull request as ready for review July 7, 2023 19:39
@alexmasi alexmasi requested a review from bormanp July 7, 2023 19:39
deploy/deploy.go Show resolved Hide resolved
deploy/deploy.go Outdated
func (d *Deployment) Deploy(ctx context.Context, kubecfg string) (rerr error) {
if d.ReportUsage {
r, err := metrics.NewReporter(ctx, d.ReportUsageProjectID, d.ReportUsageTopicID)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Put the entire thing in a method so you can say:

// reportUsage returns a function to defer by the caller.
//
//      if d.ReportUsage {
//              finish = d.reportUsage(ctx)
//              defer finish(rerr)
//      }
func (d *Deployment) reportUsage(ctx context.Context) func(error) {
        r, err := metrics.NewReporter(ctx, d.ReportUsageProjectID, d.ReportUsageTopicID)
        if err != nil {
                log.Warningf("Unable to create metrics reporter: %v", err)
                return func(_ error) {}
        }
        if id, err := r.ReportDeployClusterStart(ctx, d.event()); err != nil {
                log.Warningf("Unable to report cluster deployment start event: %v", err)
                return func(_ error) { r.Close() }
        }
        return func(rerr error) {
                if err := r.ReportDeployClusterEnd(ctx, id, rerr); err != nil {
                        log.Warningf("Unable to report cluster deployment end event: %v", err)
                }
        }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

topo/topo.go Show resolved Hide resolved
@alexmasi
Copy link
Contributor Author

alexmasi commented Jul 7, 2023

/gcbrun

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
deploy/deploy.go Show resolved Hide resolved
@alexmasi
Copy link
Contributor Author

alexmasi commented Jul 7, 2023

/gcbrun

@alexmasi
Copy link
Contributor Author

/gcbrun

@alexmasi alexmasi merged commit 2682253 into main Jul 13, 2023
6 checks passed
@alexmasi alexmasi deleted the cfg branch July 13, 2023 19:55
NehaManjunath pushed a commit that referenced this pull request Jul 14, 2023
* Add viper to read kne config

* Add config file + pubsub reporting

* Switch cli flag reading to use viper

* fix unit test

* fmt

* Controller server can report usage

* Fix unit tests for progress option

* update deploy and topo libs

* Use new metrics pkg

* add unit test

* Add section of readme explaining usage metrics reporting

* Refactor to address comments

* fix typo

* update readme
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.

2 participants