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

Fixes #25992 - pulp redirect for full disk fixed #75

Merged
merged 1 commit into from Mar 8, 2019

Conversation

lzap
Copy link
Member

@lzap lzap commented Feb 5, 2019

This fixes 302 response from pulp for ondemand sync repos.

@timogoebel
Copy link
Member

@lzap: What do you think of switching to a proper file downloading library like https://github.com/janko/down ?

@lzap
Copy link
Member Author

lzap commented Feb 5, 2019

I feel like it's probably not necessary, we only have this place and it served well. It's no rocket science I guess.

@timogoebel
Copy link
Member

@lzap: Where is the code that handles limit?

@lzap
Copy link
Member Author

lzap commented Feb 13, 2019

Whoops. As I said, this is not a rocket science ;-) Rebased.

@timogoebel
Copy link
Member

Looks good, but can we please add some tests for this?

@lzap
Copy link
Member Author

lzap commented Feb 26, 2019

Absolutely, good idea. Pulled webmock for that, ideal tool. Also copied few more Rubocop rules from core which are bugging me.

Copy link
Member

@timogoebel timogoebel left a comment

Choose a reason for hiding this comment

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

Thanks, two small comments inline.

end

test 'fetch handles redirect' do
begin
Copy link
Member

Choose a reason for hiding this comment

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

You can instead use Dir.mktmpdir do |dir| to create a temporary dir that gets automatically deleted at the end of the block.

Dir.mktmpdir do |dir|
  # ...
  ForemanBootdisk::ISOGenerator.fetch(File.join(dir, 'bootdisk-test-redirect'), url)
end

@@ -54,3 +36,10 @@ Style/ClassAndModuleChildren:
Naming/FileName:
Exclude:
- 'db/seeds.d/**/*'
# Support both ruby19 and hash_rockets
Copy link
Member

Choose a reason for hiding this comment

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

nit: missing newline

Metrics/LineLength:
Max: 180
Metrics:
Enabled: false
Copy link
Member

Choose a reason for hiding this comment

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

❤️

.rubocop.yml Outdated
@@ -54,3 +36,10 @@ Style/ClassAndModuleChildren:
Naming/FileName:
Exclude:
- 'db/seeds.d/**/*'
# Support both ruby19 and hash_rockets
Style/HashSyntax:
Enabled: false
Copy link
Member

Choose a reason for hiding this comment

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

Can we change this to EnforcedStyle: no_mixed_keys so you have to at least decide for one or the other syntax?

Copy link
Member

Choose a reason for hiding this comment

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

@lzap: This is still missing. ;-)

@lzap
Copy link
Member Author

lzap commented Mar 7, 2019

All done!

@lzap
Copy link
Member Author

lzap commented Mar 8, 2019

Fixed tests hopefully.

@timogoebel timogoebel merged commit 20789ab into theforeman:master Mar 8, 2019
@timogoebel
Copy link
Member

Thanks, @lzap.

@lzap lzap deleted the katello-redirect-25992 branch April 25, 2019 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants