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: event selection more specific - RK-19082 #76

Merged
merged 21 commits into from
Jun 29, 2023
Merged

Conversation

gosharo
Copy link
Contributor

@gosharo gosharo commented Jun 28, 2023

Signed-off-by: Gosha gosha@rookout.com

Pull Request

Description

Please provide a brief description of the changes made in this pull request.

Related Issue(s)

If this pull request addresses or relates to any open issues, please mention them here using the syntax Fixes #<issue_number> or Resolves #<issue_number>.

Checklist

Before submitting this pull request, please ensure that you have completed the following tasks:

Testing Instructions

Please provide clear instructions on how to test and verify the changes made in this pull request.

Additional Information

Add any additional information or context that would be helpful in understanding and reviewing this pull request.

Signed-off-by: Gosha <gosha@rookout.com>
@gosharo gosharo requested a review from nissim-infra as a code owner June 28, 2023 12:20
gosharo added 2 commits June 28, 2023 16:15
Signed-off-by: Gosha <gosha@rookout.com>
Signed-off-by: Gosha <gosha@rookout.com>
@gosharo
Copy link
Contributor Author

gosharo commented Jun 28, 2023

Github Enforcer opened Task: RK-19082

@sonariorobot sonariorobot changed the title fix: event selection more specific RK-19082 - fix: event selection more specific Jun 28, 2023
@gosharo gosharo changed the title RK-19082 - fix: event selection more specific fix: event selection more specific - RK-19082 Jun 28, 2023
gosharo added 4 commits June 28, 2023 20:10
Signed-off-by: Gosha <gosha@rookout.com>
Signed-off-by: Gosha <gosha@rookout.com>
Signed-off-by: Gosha <gosha@rookout.com>
Signed-off-by: Gosha <gosha@rookout.com>
@gosharo gosharo requested a review from qadiludmer June 28, 2023 17:13
func (m *MockGitProvider) GetFiles(ctx *context.Context, repo string, branch string, paths []string) ([]*git_provider.CommitFile, error) {
var commitFiles []*git_provider.CommitFile

switch repo {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use a map instead of this nested switch cases.

the map key will be repo+branch+path

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 !

gosharo added 5 commits June 29, 2023 08:34
Signed-off-by: Gosha <gosha@rookout.com>
Signed-off-by: Gosha <gosha@rookout.com>
Signed-off-by: Gosha <gosha@rookout.com>
Signed-off-by: Gosha <gosha@rookout.com>
Signed-off-by: Gosha <gosha@rookout.com>
@gosharo gosharo requested a review from qadiludmer June 29, 2023 07:30
type MockGitProvider struct{}

func GetContent(filename string) *string {
switch filename {
Copy link
Contributor

Choose a reason for hiding this comment

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

use map also here

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


func (m *MockGitProvider) GetFile(ctx *context.Context, repo string, branch string, path string) (*git_provider.CommitFile, error) {
fileMappings := *GetFileMap()
if branchMap, ok := fileMappings[repo]; ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

you can flatten the map so you will only have to check single key that is "repo/branch/path"

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


func GetFileMap() *map[string]map[string]map[string]git_provider.CommitFile {
return &map[string]map[string]map[string]git_provider.CommitFile{
"repo1": {
Copy link
Contributor

Choose a reason for hiding this comment

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

flatten it

return &map[string]git_provider.CommitFile{
    "repo1/branch1/.workflows/main.yaml": git_provider.CommitFile{
			Path:    utils.SPtr(".workflows/main.yaml"),
			Content: GetContent(".workflows/main.yaml"),
		},
    "repo1/branch1/.workflows/exit.yaml": git_provider.CommitFile{
			Path:    utils.SPtr(".workflows/exit.yaml"),
			Content: GetContent(".workflows/exit.yaml"),
		},
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@qadiludmer not sure that it's better, is much easier to read and add new repos, branches and files in structured map

Copy link
Contributor

Choose a reason for hiding this comment

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

But the access is less pretty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, but I have the GetFile and GetFiles function for that

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

Copy link
Contributor

@qadiludmer qadiludmer left a comment

Choose a reason for hiding this comment

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

see comments

@qadiludmer
Copy link
Contributor

Give meaningful names to commits to make the review easier :

image

gosharo added 3 commits June 29, 2023 12:43
Signed-off-by: Gosha <gosha@rookout.com>
Signed-off-by: Gosha <gosha@rookout.com>
Signed-off-by: Gosha <gosha@rookout.com>
@gosharo gosharo requested a review from qadiludmer June 29, 2023 09:50
Signed-off-by: Gosha <gosha@rookout.com>
return fileContentMap[filename]
}

func GetFileMap() *map[string]*git_provider.CommitFile {
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need a function for retuning a const ? can't we put the map in a global var ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hate globals

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also it can be reused in other places too, if needed

Copy link
Contributor

Choose a reason for hiding this comment

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

Function contains a logic - you don't have any logic here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes... after reading a little, moving it to global

Copy link
Contributor

Choose a reason for hiding this comment

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

var fileContentMap := map[string]*string{
		"main.yaml":       utils.SPtr("main.yaml"),
		"exit.yaml":       utils.SPtr("exit.yaml"),
		"parameters.yaml": utils.SPtr("parameters.yaml"),
	}

Copy link
Contributor

@qadiludmer qadiludmer Jun 29, 2023

Choose a reason for hiding this comment

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

and then

var commitFileMap := map[string]*git_provider.CommitFile{
		"repo1/branch1/.workflows/main.yaml": &git_provider.CommitFile{
			Path:    utils.SPtr(".workflows/main.yaml"),
			Content: fileContentMap["main.yaml"],
		},
....
	}

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

Copy link
Contributor

Choose a reason for hiding this comment

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

almost. see my second comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

did it, and renamed it

gosharo added 3 commits June 29, 2023 15:28
Signed-off-by: Gosha <gosha@rookout.com>
Signed-off-by: Gosha <gosha@rookout.com>
Signed-off-by: Gosha <gosha@rookout.com>
@gosharo gosharo requested a review from qadiludmer June 29, 2023 12:40
"parameters.yaml": utils.SPtr("parameters.yaml"),
}

func GetContent(filename string) *string {
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you need this function ? all it does is accessing a map so the caller can do it too

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

Copy link
Contributor

@qadiludmer qadiludmer left a comment

Choose a reason for hiding this comment

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

see comment

gosharo added 2 commits June 29, 2023 16:45
Signed-off-by: Gosha <gosha@rookout.com>
Signed-off-by: Gosha <gosha@rookout.com>
@gosharo gosharo requested a review from qadiludmer June 29, 2023 13:46
@gosharo gosharo merged commit db58bb3 into main Jun 29, 2023
@gosharo gosharo deleted the fix-event-selection branch June 29, 2023 16:43
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