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

improve (azure-pipelines): log url on error #2297

Merged
merged 6 commits into from
Nov 23, 2021

Conversation

endersonmenezes
Copy link
Contributor

@endersonmenezes endersonmenezes commented Nov 17, 2021

Signed-off-by: Enderson Menezes endersonster@gmail.com

Logging URL on Azure Pipelines Scaler Error!

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • Changelog has been updated

Hi, I'm new to Go and here at Keda we are running a POC within the company and this little addition helped us to better understand our mistake.

Does this change need to change CHANGELOG? Any other nonsense I've done let me know please!

Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

Thanks a ton for your contribution!
Only small nit

pkg/scalers/azure_pipelines_scaler.go Outdated Show resolved Hide resolved
@endersonmenezes
Copy link
Contributor Author

endersonmenezes commented Nov 17, 2021

Thanks a ton for your contribution! Only small nit

I'm going to look for more simple errors like that, and it helps me understand the code better.

Edit: I make error with Signature, i will correct then.

@JorTurFer
Copy link
Member

Thanks a ton for your contribution! Only small nit

I'm going to look for more simple errors like that, and it helps me understand the code better.

Edit: I make error with Signature, i will correct then.

You need to amend the wrong commit, adding other one signed it's not enough.
You can see in the docs how to do it:
https://github.com/kedacore/keda/blob/main/CONTRIBUTING.md#i-didnt-sign-my-commit-now-what

…rado Ferrero <Jorge_turrado@hotmail.es>

Signed-off-by: Enderson Menezes <endersonster@gmail.com>
@endersonmenezes
Copy link
Contributor Author

Thanks a ton for your contribution! Only small nit

I'm going to look for more simple errors like that, and it helps me understand the code better.
Edit: I make error with Signature, i will correct then.

You need to amend the wrong commit, adding other one signed it's not enough. You can see in the docs how to do it: https://github.com/kedacore/keda/blob/main/CONTRIBUTING.md#i-didnt-sign-my-commit-now-what

Like that?

pkg/scalers/azure_pipelines_scaler.go Outdated Show resolved Hide resolved
@JorTurFer
Copy link
Member

Thanks a ton for your contribution! Only small nit

I'm going to look for more simple errors like that, and it helps me understand the code better.
Edit: I make error with Signature, i will correct then.

You need to amend the wrong commit, adding other one signed it's not enough. You can see in the docs how to do it: https://github.com/kedacore/keda/blob/main/CONTRIBUTING.md#i-didnt-sign-my-commit-now-what

Like that?

Yes, exactly like that :)

Thanks a ton!

Signed-off-by: Enderson Menezes <endersonster@gmail.com>
@JorTurFer
Copy link
Member

JorTurFer commented Nov 17, 2021

There is an error yet, could you take a look at it please? 🙏
https://github.com/kedacore/keda/runs/4242581591?check_suite_focus=true
Basically, you should change string(url) to this url on both lines. The casting to string is not necessary because url is already string

Signed-off-by: Enderson Menezes <endersonster@gmail.com>
@endersonmenezes
Copy link
Contributor Author

Sorry, i will install pre-commit and take more attention.

@JorTurFer
Copy link
Member

JorTurFer commented Nov 17, 2021

Sorry, i will install pre-commit and take more attention.

Don't worry, It happens :)

sorry for this because I didn't see it before:
Error messages in golang shouldn't start with capital letters and that's the error that there is now. :(

Apart from this, personally, I use dev containers in VS Code to avoid installing dependencies in my computer. KEDA supports it and pre-commit is already installed there. I prefer that than installing all dependencies for each project, maybe you'd like it

BTW, thanks for the contribution ❤️

Signed-off-by: Enderson Menezes <endersonster@gmail.com>
@endersonmenezes
Copy link
Contributor Author

Sorry, i will install pre-commit and take more attention.

Don't worry, It happens :)

sorry for this because I didn't see it before: Error messages in golang shouldn't start with capital letters and that's the error that there is now. :(

Apart from this, personally, I use dev containers in VS Code to avoid installing dependencies in my computer. KEDA supports it and pre-commit is already installed there. I prefer that than installing all dependencies for each project, maybe you'd like it

BTW, thanks for the contribution heart

i will read this

Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

Thanks ❤️❤️❤️

Copy link
Member

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution, added a question and would you mind updating the changelog please?

pkg/scalers/azure_pipelines_scaler.go Outdated Show resolved Hide resolved
Signed-off-by: Enderson Menezes <endersonster@gmail.com>
Signed-off-by: Enderson Menezes <endersonster@gmail.com>
@endersonmenezes
Copy link
Contributor Author

I don't understand why the "CI / Static Checks (pull_request) " failed :(

@JorTurFer
Copy link
Member

I don't understand why the "CI / Static Checks (pull_request) " failed :(

I think that it was a random fail :(

@zroubalik zroubalik merged commit cbe929b into kedacore:main Nov 23, 2021
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.

4 participants