PMM-4879 Adding support for defaults-file in new mysql service. by pkadej · Pull Request #1068 · percona/pmm-managed

pkadej · 2022-04-12T21:30:18Z

pmm

it-percona-cla · 2022-04-12T21:30:22Z

All committers have signed the CLA.

BupycHuk · 2022-04-20T10:36:04Z

+)
+
+// ParseDefaultsFile requests from agent to parse defaultsFile.
+type ParseDefaultsFile struct {


maybe DefaultsFileParser will be better naming for this struct?

Sure, fixed.

BupycHuk · 2022-04-20T10:43:31Z

+	if !ok {
+		return nil, errors.New("wrong response from agent (not ParseDefaultsFileResponse model)")
+	}
+	if len(parserResponse.GetError()) != 0 {


let's use field instead of method, we had to use methods in some places to not duplicate code

in a cases below, please use fields too

BupycHuk · 2022-04-20T10:45:00Z

 			return err
 		}
+
+		if len(req.DefaultsFile) != 0 {


Suggested change

if len(req.DefaultsFile) != 0 {

if req.DefaultsFile != "" {

the same for cases below

BupycHuk · 2022-04-20T19:16:48Z

+			if len(result.Username) != 0 {
+				req.Username = result.Username
+			}
+			if len(result.Password) != 0 {
+				req.Password = result.Password
+			}
+
+			if len(result.Host) != 0 {
+				req.Address = result.Host
+			}
+
+			if result.Port > 0 {
+				req.Port = result.Port
+			}


based on discussion in Jira ticket we should set values from defaults file only if they weren't passed as a separate field in request.

BupycHuk · 2022-04-20T19:17:15Z

 	state agentsStateUpdater
 	cc    connectionChecker
 	vc    versionCache
+	pfd   defaultsFileParser


Suggested change

pfd defaultsFileParser

dfp defaultsFileParser

Refactored.

BupycHuk · 2022-04-23T20:21:29Z

@pkadej could you please commit changes in go modules, so we can see result of CI.

pkadej · 2022-04-24T15:40:09Z

@pkadej could you please commit changes in go modules, so we can see result of CI.
@BupycHuk Did you mean temporary updating mod.go with pmm replaced with my fork or with version after percona/pmm#863 being merged?

BupycHuk · 2022-04-25T08:47:30Z

@pkadej yes, updated go.mod with replace directive for now and once PR in pmm is merged update go.mod to use that version.

pkadej · 2022-04-25T19:18:43Z

@pkadej yes, updated go.mod with replace directive for now and once PR in pmm is merged update go.mod to use that version.

@BupycHuk
Done. Unfortunately i don't have permission to trigger runflow.

codecov · 2022-04-26T19:17:12Z

Codecov Report

Merging #1068 (5267092) into main (05124a1) will increase coverage by 0.30%.
The diff coverage is 42.62%.

@@            Coverage Diff             @@
##             main    #1068      +/-   ##
==========================================
+ Coverage   48.67%   48.98%   +0.30%     
==========================================
  Files         181      182       +1     
  Lines       21264    21323      +59     
==========================================
+ Hits        10351    10445      +94     
+ Misses       9775     9723      -52     
- Partials     1138     1155      +17

Flag	Coverage Δ
all	`48.64% <42.62%> (+0.30%)`	⬆️
cover	`49.14% <42.62%> (+0.33%)`	⬆️
crosscover	`48.98% <42.62%> (+0.30%)`	⬆️
update	`12.84% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
main.go	`0.00% <0.00%> (ø)`
services/agents/parse_defaults_file.go	`20.00% <20.00%> (ø)`
services/agents/channel/channel.go	`62.28% <100.00%> (+0.43%)`	⬆️
services/management/mysql.go	`54.03% <100.00%> (+54.03%)`	⬆️
services/management/management.go	`28.26% <0.00%> (+18.47%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05124a1...5267092. Read the comment docs.

BupycHuk · 2022-04-28T21:40:49Z

 	<-stop
 }
+
+func TestChannelForDefaultsFileParser(t *testing.T) {


I'm not sure what are we testing here

ritbl · 2022-05-02T09:33:39Z

+		}
+		return request, nil
+	} else {
+		return nil, errors.Errorf("unhandled service type %s", serviceType)


Suggested change

return nil, errors.Errorf("unhandled service type %s", serviceType)

return nil, errors.Errorf("unsupported service type %s", serviceType)

ritbl · 2022-05-02T09:36:51Z

+	start := time.Now()
+	defer func() {
+		if dur := time.Since(start); dur > 5*time.Second {
+			l.Warnf("ParseDefaultsFile took %s.", dur)
+		}
+	}()


Is it realistic? Reading a file with few lines should not be a problem. And, I think, it should be handed in agent

That can be written like this:

defer func(start time.Time) { if dur := time.Since(start); dur > 5*time.Second { l.Warnf("ParseDefaultsFile took %s.", dur) } }(time.Now())

But yes, I also don't see benefit of this log

ritbl · 2022-05-02T09:37:56Z

+	l.Infof("ParseDefaultsFile response from agent: %+v.", resp)
+	parserResponse, ok := resp.(*agentpb.ParseDefaultsFileResponse)
+	if !ok {
+		return nil, errors.New("wrong response from agent (not ParseDefaultsFileResponse model)")


Can we add info regarding actual type?

ritbl · 2022-05-02T09:42:25Z

+				return status.Error(codes.FailedPrecondition, fmt.Sprintf("Defaults file error: %s.", err))
+			}
+
+			// set username and password from parsed defaults file by agent


nit: it would be good to know that value has been overridden, can we log.debug that?

artemgavrilov

LGTM with comments

artemgavrilov · 2022-05-05T11:30:38Z

+	start := time.Now()
+	defer func() {
+		if dur := time.Since(start); dur > 5*time.Second {
+			l.Warnf("ParseDefaultsFile took %s.", dur)
+		}
+	}()


That can be written like this:

defer func(start time.Time) { if dur := time.Since(start); dur > 5*time.Second { l.Warnf("ParseDefaultsFile took %s.", dur) } }(time.Now())

But yes, I also don't see benefit of this log

artemgavrilov · 2022-05-05T14:10:09Z

+		return nil, err
+	}
+
+	l.Infof("ParseDefaultsFile response from agent: %+v.", resp)


I think Debug level fits better here.

artemgavrilov · 2022-05-05T14:30:19Z

+		if req.DefaultsFile != "" {
+			result, err := s.dfp.ParseDefaultsFile(ctx, req.PmmAgentId, req.DefaultsFile, models.MySQLServiceType)
+			if err != nil {
+				return status.Error(codes.FailedPrecondition, fmt.Sprintf("Defaults file error: %s.", err))


Suggested change

return status.Error(codes.FailedPrecondition, fmt.Sprintf("Defaults file error: %s.", err))

return status.Errorf(codes.FailedPrecondition, "Defaults file error: %s.", err)

artemgavrilov · 2022-05-05T14:36:33Z

+	versionCache := &mockVersionCache{}
+	versionCache.Test(t)
+
+	teardown := func(t *testing.T) {


You can use t.Cleanup instead.

PMM-4879 Adding support for defaults-file in new mysql service.

860c132

pkadej and others added 3 commits April 15, 2022 02:50

PMM-4879 defaults-file adjustments.

2f7f34d

Merge branch 'main' into PMM-4879-defaults-file

2168c85

PMM-4879 Linter & tests fix.

f6c7690

pkadej marked this pull request as ready for review April 20, 2022 09:44

pkadej requested review from BupycHuk and artemgavrilov as code owners April 20, 2022 09:44

BupycHuk suggested changes Apr 20, 2022

View reviewed changes

pkadej added 2 commits April 21, 2022 06:42

PMM-4879 Adding support for configuration entry: socket.

ea8e53e

PMM-4879 Code review adjustments.

67e20e5

pkadej requested a review from BupycHuk April 23, 2022 13:03

Merge branch 'main' into PMM-4879-defaults-file

6964cfd

pkadej and others added 2 commits April 25, 2022 20:23

PMM-4879 Code review adjustments: replacing API dependencies.

5f78aa2

Merge branch 'main' into PMM-4879-defaults-file

53bb457

pkadej and others added 3 commits April 26, 2022 20:20

PMM-4879 Code review adjustments: replacing API dependencies.

868c96b

PMM-4879 Code review adjustments: fixing code for CI.

1d179ca

Merge branch 'main' into PMM-4879-defaults-file

031c091

pkadej added 2 commits April 27, 2022 19:26

Merge branch 'main' into PMM-4879-defaults-file

5cb6346

PMM-4879 Added coverage tests.

64538ce

pkadej force-pushed the PMM-4879-defaults-file branch from b4c8c0a to 64538ce Compare April 27, 2022 17:56

pkadej added 3 commits April 27, 2022 23:04

PMM-4879 Added coverage tests.

2b875c8

PMM-4879 Code review adjustments.

9889453

Merge branch 'main' into PMM-4879-defaults-file

bdd11c2

BupycHuk approved these changes Apr 28, 2022

View reviewed changes

ritbl approved these changes May 2, 2022

View reviewed changes

Merge branch 'main' into PMM-4879-defaults-file

469b7bc

pkadej mentioned this pull request May 3, 2022

PMM-4879 Adding support for defaults-file in new mysql service. #1103

Closed

PMM-4879 Updating api-tests.

f0d0326

artemgavrilov approved these changes May 5, 2022

View reviewed changes

artemgavrilov assigned pkadej May 5, 2022

pkadej mentioned this pull request Jun 9, 2022

PMM-4879 mysql add defaults file percona/pmm#928

Merged

Merge branch 'main' into PMM-4879-defaults-file

5267092

pkadej merged commit 1011fab into percona:main Jun 11, 2022

	return nil, errors.Errorf("unhandled service type %s", serviceType)
	return nil, errors.Errorf("unsupported service type %s", serviceType)

	return status.Error(codes.FailedPrecondition, fmt.Sprintf("Defaults file error: %s.", err))
	return status.Errorf(codes.FailedPrecondition, "Defaults file error: %s.", err)

Uh oh!

Conversation

pkadej commented Apr 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

it-percona-cla commented Apr 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BupycHuk commented Apr 23, 2022

Uh oh!

pkadej commented Apr 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BupycHuk commented Apr 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pkadej commented Apr 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Apr 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artemgavrilov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pkadej commented Apr 12, 2022 •

edited

Loading

it-percona-cla commented Apr 12, 2022 •

edited

Loading

pkadej commented Apr 24, 2022 •

edited

Loading

BupycHuk commented Apr 25, 2022 •

edited

Loading

pkadej commented Apr 25, 2022 •

edited

Loading

codecov Bot commented Apr 26, 2022 •

edited

Loading