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

replace progress message #412

Merged
merged 1 commit into from
Jun 26, 2024
Merged

replace progress message #412

merged 1 commit into from
Jun 26, 2024

Conversation

mohamedasifs123
Copy link
Contributor

closes #333

@mohamedasifs123 mohamedasifs123 requested a review from a team as a code owner June 22, 2024 16:14
Copy link
Member

@glimchb glimchb left a comment

Choose a reason for hiding this comment

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

This is not right
Each place that calls function need to pass it’s own message

@Xeckt
Copy link
Contributor

Xeckt commented Jun 24, 2024

I'm not a reviewer by any means but a small nitpick about the commits, try and describe in a short way what you've done with the commit. All your commit messages are the same so it would be hard to discern which is which at a later time if need be.

@mohamedasifs123

@glimchb
Copy link
Member

glimchb commented Jun 24, 2024

looks better, thanks for the contribution @mohamedasifs123
please fix CI issues

@glimchb
Copy link
Member

glimchb commented Jun 25, 2024

  1. golangci is failing on Error: File is notgofmt-ed with -s (gofmt)
  2. commitliont is failing (i think on the first commit) - you can squash all commits now and fix the message

@mohamedasifs123
Copy link
Contributor Author

I will do squash commit

@glimchb
Copy link
Member

glimchb commented Jun 25, 2024

@mohamedasifs123 looks good, please squash so we pass Commitlint checker

Also please consider moving doReportProgress() to https://github.com/opiproject/sztp/blob/main/sztp-agent/pkg/secureagent/progress.go

@mohamedasifs123
Copy link
Contributor Author

Okay @glimchb
I will move the function.

@glimchb
Copy link
Member

glimchb commented Jun 26, 2024

@mohamedasifs123 great work, looks correct to me now...

please add space between feat: message in the commit message of both commits to pass commitlint and we can merge this

feat:moving the doProgressReport -> feat: moving the doProgressReport

maybe also message need to be lowercase - don't remember... you can try commitlint locally...

Signed-off-by: mohamedasifs123 <[email protected]>
@mohamedasifs123
Copy link
Contributor Author

Yeah, thanks for suggesting local check of commitlint

Copy link
Member

@glimchb glimchb left a comment

Choose a reason for hiding this comment

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

great, ready for merge

@glimchb glimchb added the Merge Candidate in the open merge window, next candidate for merge label Jun 26, 2024
@glimchb glimchb merged commit e61f053 into opiproject:main Jun 26, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merge Candidate in the open merge window, next candidate for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

replace progress message message sent via JSON with something more meaningful
3 participants