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.

Sign up for GitHub

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

Jump to bottom

[webui][api] Update nokogiri gem. #3902

Closed
wants to merge 2 commits into from
Closed

[webui][api] Update nokogiri gem. #3902

wants to merge 2 commits into from

Conversation

evanrolfe
Copy link

There is a security issue with libxml2 which is used by nokogiri.
(CVE-2017-0663).

@evanrolfe evanrolfe added the Frontend Things related to the OBS RoR app label Sep 25, 2017
mschnitzer
mschnitzer approved these changes Sep 25, 2017
View reviewed changes
Copy link
Member

@mschnitzer mschnitzer left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Ana06
Ana06 suggested changes Sep 26, 2017
View reviewed changes
src/api/.rubocop.yml Outdated
@@ -141,6 +142,9 @@ Style/TernaryParentheses:
Style/ZeroLengthPredicate:
Enabled: true

Style/FrozenStringLiteralComment:
Copy link
Member

Choose a reason for hiding this comment

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

why are you disabling this cop and how is this related to the update? 🤔

Copy link
Author

Choose a reason for hiding this comment

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

I have no idea whats going on with rubocop in this PR... It was telling me to add this comment to a file:
# frozen_string_literal: true

But that also made some specs fail too...

@Ana06
Copy link
Member

Ana06 commented Sep 26, 2017 •
edited
Loading

Why does our test suite look different? I mean, linters are now separated, and where is git-cop? 😕 😖

@evanrolfe
Copy link
Author

Why does our test suite look different? I mean, linters are now separated, and where is git-cop? 😕 😖

Because this is 2.8, before the linters ran first and before we had git-cop..

@bgeuken @mdeniz it seems that rubocop is checking files that were not changed in this PR which is preventing this from being merged. Can you please have a look at this? I'm not really sure what to do about it..

@Ana06
Copy link
Member

Ana06 commented Sep 26, 2017

@evanrolfe

Because this is 2.8, before the linters ran first and before we had git-cop..

right! 😉

@evanrolfe

@bgeuken @mdeniz it seems that rubocop is checking files that were not changed in this PR which is preventing this from being merged. Can you please have a look at this? I'm not really sure what to do about it..

From Rubocop documentation:

This cop is designed to help upgrade to Ruby 3.0. It will add the comment # frozen_string_literal: true to the top of files to enable frozen string literals. Frozen string literals will be default in Ruby 3.0. The comment will be added below a shebang and encoding comment. The frozen string literal comment is only valid in Ruby 2.3+.

So, I think the problem is that you aded TargetRubyVersion: 2.4

@evanrolfe
Copy link
Author

So, I think the problem is that you aded TargetRubyVersion: 2.4

Ok I removed TargetRubyVersion and now we get the original rubocop failure which caused me to add TargetRubyVersion in..

spec/controllers/webui/package_controller_spec.rb:344:20: E: unexpected token tLSHFT
(Using Ruby 2.1 parser; configure using TargetRubyVersion parameter, under AllCops)
        expected = <<~EOT

@Ana06
Copy link
Member

Ana06 commented Sep 26, 2017

There is a problem with the versions, because I think the Ruby version we are using to run Rubocop is not the same as the one run. That's why when you add TargetRubyVersion: 2.4 the offenses change.

There is an issue in Rubocop: rubocop/rubocop#2956

This is closed, so it is supposed to be fixed, I'll write there or open a new issue because it seems it is not. We are not the only ones having problems with that, as if you check in the issue there are some people in the last months pointing this issue with similar issues.

So, until they fix it, I would stuck Rubocop version, as you did before but maybe taking 2.4.1 (TargetRubyVersion: 2.4.1) and we may want to correct the offenses instead of disable the cop or we will have to do it if at some point we want to update to Ruby 3.0. This cop support autocorrect, so you can just run:

bundle exec rubocop --auto-correct --only FrozenStringLiteralComment

Ana06
Ana06 reviewed Sep 26, 2017
View reviewed changes
src/api/.rubocop.yml Outdated
@@ -1,6 +1,7 @@
inherit_from: .rubocop_todo.yml

AllCops:
TargetRubyVersion: 2.4.1
Copy link
Member

Choose a reason for hiding this comment

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

@evanrolfe sorry, we are installing Ruby 2.4.0, so this should be TargetRubyVersion: 2.4

Can this and the regeneration of the todo file be in a different commit?

@evanrolfe
[webui][api] Update nokogiri gem.
db66bde 
There is a security issue with libxml2 which is used by nokogiri.
(CVE-2017-0663).
@evanrolfe
[ci] Disable rubocop from travis.
dbe2097 
We are having too many problems trying to get rubocop to pass
and since this is a stable 2.8 branch which doesn't have much being
developed on we have decided to just disable it and fix in master.
@bgeuken
Copy link
Member

bgeuken commented Sep 26, 2017

I don't get why we start having all these rubocops failing out of a sudden. I would prefer not to disable the whole rubocop test suite. Though after trying to disable the failing cop and having other cops fail instead I am okay to disable this for now.

Ana06
Ana06 suggested changes Sep 27, 2017
View reviewed changes
Copy link
Member

@Ana06 Ana06 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

DO NOT DISABLE THE WHOLE RUBOCOP! 😡 😭 and BTW Rubocop is still failling. I guess you did something wrong when disabling it @evanrolfe.

I fixed the problem: https://travis-ci.org/Ana06/open-build-service/builds/280285463 That branch include @evanrolfe fix and fix Rubocop. The Test suite is completely green 💚 I can make a PR that supersedes this want if you want @evanrolfe and we can merge that. 😉

@evanrolfe
Copy link
Author

evanrolfe commented Sep 27, 2017 •
edited
Loading

DO NOT DISABLE THE WHOLE RUBOCOP! 😡 😭

I don't see the difference is but OK!!!! 😆

I can make a PR that supersedes this want if you want @evanrolfe and we can merge that. 😉

Yes, please do this! Thank you!

@Ana06 Ana06 mentioned this pull request Sep 27, 2017
Merged
@Ana06
Copy link
Member

Ana06 commented Sep 27, 2017

@evanrolfe

I can make a PR that supersedes this want if you want @evanrolfe and we can merge that. 😉

Yes, please do this! Thank you!

Done! #3910 😉

@Ana06 Ana06 closed this Sep 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ana06 Ana06 Ana06 requested changes

@mschnitzer mschnitzer mschnitzer approved these changes

@bgeuken bgeuken bgeuken approved these changes

Assignees
No one assigned
Labels
Frontend Things related to the OBS RoR app
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@evanrolfe @Ana06 @bgeuken @mschnitzer