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: wrong references crashes library #180

Merged
merged 13 commits into from
May 7, 2021

Conversation

jonaslagoni
Copy link
Sponsor Member

@jonaslagoni jonaslagoni commented May 6, 2021

Description
This PR improves the maintainability for input processosr by only moving from integration test to unit test as well as fixing a bug that crashes the library if references are wrong

Related issue(s)
Related to #64

@jonaslagoni jonaslagoni changed the title chore: improved maintainability for JSON schema input processor fix: wrong references crashes library May 6, 2021
@jonaslagoni
Copy link
Sponsor Member Author

jonaslagoni commented May 6, 2021

@derberg I would really like for coveralls to give me an indication here, to ensure that the coverage did not drop with these test changes. (I checked manually, but reviewer have no idea)

Can we try switching from trigger pull_request_target to pull_request? 🤔

@coveralls
Copy link

coveralls commented May 6, 2021

Coverage Status

Coverage increased (+0.06%) to 94.645% when pulling 97e1098 on jonaslagoni:feature/rework_process_test into e41ca23 on asyncapi:master.

@jonaslagoni
Copy link
Sponsor Member Author

jonaslagoni commented May 6, 2021

Alright then, coveralls are just a 🐢.

@derberg
Copy link
Member

derberg commented May 7, 2021

It works 🎉
🤝 congrats

Happy with the solution so far? I guess we need a couple of weeks of testing before we roll it out to others?

Check test coverage / Build should be made mandatory.

if we would have pull_request, the bot would have no right to make the comment that you can see above

@sonarcloud
Copy link

sonarcloud bot commented May 7, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
2.1% 2.1% Duplication

Copy link
Member

@magicmatatjahu magicmatatjahu left a comment

Choose a reason for hiding this comment

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

LGTM :)

@jonaslagoni
Copy link
Sponsor Member Author

if we would have pull_request, the bot would have no right to make the comment that you can see above

@derberg I played around with switching between pull_request and pull_request_target. And the comment is not updated (the bot update the comment when commits are pushed) when we are using pull_request_target. In order for the bot to write on PR's we have to use pull_request. As I have switched to currently.

@derberg
Copy link
Member

derberg commented May 7, 2021

And the comment is not updated (the bot update the comment when commits are pushed)

I don't get it, you mean the comment from coveralls bot showed up only when you were using pull_request?

@jonaslagoni
Copy link
Sponsor Member Author

jonaslagoni commented May 7, 2021

I don't get it, you mean the comment from coveralls bot showed up only when you were using pull_request?

@derberg Yes, I have switched back and forth figuring out when it updates the comment. I tried to switch here f0514ed
and I assume that only then the bot comment showed up. Also why use pull_request_target if the user can switch to pull_request and it runs anyway 🤷

@derberg
Copy link
Member

derberg commented May 7, 2021

it runs, it will be, but GITHUB_TOKEN has only read rights as when user changes to pull_request then the workflow file is taken from the fork, and token has read rights only to not enable token leaking. pull_request_target means that on PR, workflow is taken from master (default branch) and therefore token has more privileges as it cannot be leaked.

This is why it is super confusing for me, that pull_request works and bot makes comment 🤷🏼 magic 😄 or the coveralls bot just like sonarcloud bot are completely separate from the workflow, they work independently. So yeah, if pull_request works, then just merge 😄 and I put this case into my x-files box of unsolved cases and someday talk to Mulder and Scully to help me out figuring out what was wrong 😆

@jonaslagoni jonaslagoni merged commit 0d0f32f into asyncapi:master May 7, 2021
@jonaslagoni jonaslagoni deleted the feature/rework_process_test branch May 7, 2021 12:28
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 0.6.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants