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

[security] feature add or-option #1151

Merged
merged 4 commits into from
Mar 8, 2022
Merged

Conversation

Lehp
Copy link
Contributor

@Lehp Lehp commented Feb 28, 2022

Describe the PR
Feature to generate OR-security option for swagger files.

Relation issue
e.g. https://github.com/swaggo/swag/pull/118/files

Additional context
Sorry for not waiting for any approval Was watching at the source code and almost accidentally coded this.

I desperately need this function to make this repo useful for my company.

@ubogdan
Copy link
Contributor

ubogdan commented Mar 3, 2022

Please fix the unit tests to go forward with a code review.

@Lehp
Copy link
Contributor Author

Lehp commented Mar 4, 2022

Please fix the unit tests to go forward with a code review.

Hey ubogdan thx for your res.
I must admit I just learned how to code in GO a month ago. So i am very confused with this library.
Somehow the tests concerning my change worked. I had a permission denied error in TestWithMonkeyFilepathAbs test. I believe it to be a local Problem but don't know what it means and it's quite hard for me to debug this code. Build is green after run fmt commit but I'm suspicious. Is everythink ok or where and what is the problem rn?

Copy link
Contributor

@ubogdan ubogdan 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.

operation.go Outdated
return nil
}

func (operation *Operation) ProcessSecurityOption(securityOption string, securityMap SecurityMap) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need additional export. Even make this a private function even move the code inside the for loop.

operation.go Outdated
@@ -629,30 +629,38 @@ func (operation *Operation) ParseRouterComment(commentLine string) error {
return nil
}

type SecurityMap map[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.

Useless definition. Please remove.

@ubogdan
Copy link
Contributor

ubogdan commented Mar 7, 2022

unit tests are failing...

@Lehp
Copy link
Contributor Author

Lehp commented Mar 8, 2022

I'm having file-permission errors running tests locally. Is it a git convention that tests on the repo only run when someone approves the workflow?

@ubogdan
Copy link
Contributor

ubogdan commented Mar 8, 2022

@Lehp Workflow approval is required because you are first time contributor on this repo.

@codecov
Copy link

codecov bot commented Mar 8, 2022

Codecov Report

Merging #1151 (750be1e) into master (faad956) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1151      +/-   ##
==========================================
+ Coverage   94.73%   94.76%   +0.02%     
==========================================
  Files          10       10              
  Lines        2433     2443      +10     
==========================================
+ Hits         2305     2315      +10     
  Misses         67       67              
  Partials       61       61              
Impacted Files Coverage Δ
operation.go 95.98% <100.00%> (+0.06%) ⬆️

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 faad956...750be1e. Read the comment docs.

@Lehp
Copy link
Contributor Author

Lehp commented Mar 8, 2022

@ubogdan do you know why this tests fails locally for me?
gomonkey.ApplyFunc panics for me and I don't know why.
image

@ubogdan
Copy link
Contributor

ubogdan commented Mar 8, 2022

See notes :
A panic may happen when a goroutine is patching a function or a member method that is visited by another goroutine at the same time. That is to say, gomonkey is not threadsafe.

Copy link
Contributor

@ubogdan ubogdan left a comment

Choose a reason for hiding this comment

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

LGTM

@ubogdan ubogdan merged commit 6686f54 into swaggo:master Mar 8, 2022
@ubogdan
Copy link
Contributor

ubogdan commented Mar 8, 2022

I just realized the README.nd wasn't updated so that the users can benefit from this feature. Would you mind creating another PR with the documentation update ?.

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.

3 participants