Skip to content

update to feedjira 3#535

Merged
mockdeep merged 1 commit into
stringer-rss:masterfrom
dperson:update-feedjira
Feb 25, 2021
Merged

update to feedjira 3#535
mockdeep merged 1 commit into
stringer-rss:masterfrom
dperson:update-feedjira

Conversation

@dperson
Copy link
Copy Markdown
Contributor

@dperson dperson commented Feb 23, 2021

This is a functional and tested version of #472
The 2.x version HEAD is on has been EOL since 2018.

Comment thread Gemfile
gem "feedbag", "~> 0.9.5"
gem "feedjira", "~> 2.1.3"
gem "feedjira", "~> 3.0"
gem "httparty"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It's not a huge deal, but I wonder if we'd be better off using Ruby's built in Net::HTTP library rather than adding a new dependency. I know Net::HTTP doesn't have the greatest reputation for usability, but we're only making a couple of requests so it doesn't seem like it'll make a huge difference. I'll leave it up to you, but my bias is towards minimizing dependencies unless they save us a ton of time and headache.

@dperson dperson force-pushed the update-feedjira branch 5 times, most recently from 15b6a22 to 0c4ea90 Compare February 24, 2021 03:00
@dperson
Copy link
Copy Markdown
Contributor Author

dperson commented Feb 24, 2021

Okay, I had just gone with the suggested solution from the feedjira documentation and examples, but that is a completely valid point. I've made the changes and fixed the CI tests.

Let me push the change to my personal instance and make sure it doesn't break horribly.

@dperson
Copy link
Copy Markdown
Contributor Author

dperson commented Feb 24, 2021

Hmm, I think that it's not getting all of my feeds any longer (with Net::HTTP). There are a few that I normally get updates every morning from that I haven't seen, and I have a lot more amber and red feeds in the feed viewer.

My experience with Ruby (besides this project) was writing some extensions for Puppet to pull in information from internal corporate sources for configuring hosts... I really don't know where to go to try and debug / fix this issue.

Unless you have some pointers, I'm inclined to go back to their suggested HTTP client...

Sometimes working / sometimes not is my least favorite outcome. 😢

@dperson
Copy link
Copy Markdown
Contributor Author

dperson commented Feb 24, 2021

Yeah after reverting to HTTParty my feeds that I wasn't seeing updates on all came in. I don't know why it didn't work with Net::HTTP. Let me know if you're okay with changing this pull request back to match, or if more work needs to be done on getting Net:HTTP to work? Thanks.

@mockdeep
Copy link
Copy Markdown
Collaborator

@dperson are you able to share one or two of the feeds that didn't work for you? I'll see if I can figure out the logging situation and get a better picture of what is going wrong. Could be we're better off with HTTParty, but I'd like to understand what is happening first.

@dperson
Copy link
Copy Markdown
Contributor Author

dperson commented Feb 24, 2021

Two that I can remember coming in immediately after swapping back to HTTParty are:

http://www.quotationspage.com/data/qotd.rss
http://www.sffaudio.com/?feed=rss2

Thanks!

@mockdeep
Copy link
Copy Markdown
Collaborator

Okay, so I'm poking around with it and for some reason Net::HTTP.get returns an empty string for the second feed, but HTTParty.get returns an XML document as expected. That's just weird behavior, so I guess we'd better stick with HTTParty. Sorry, another flip flop!

I'm not sure if it's a Feedjira thing or a Net::HTTP thing, but I wasn't able to add your qotd feed on your branch. I am able to add it on master, though. If I click the link it redirects me to a Feedburner page which I am able to add as a feed on your branch.

@mockdeep
Copy link
Copy Markdown
Collaborator

Also, maybe you're already aware of it, but you can test these things out locally so you don't risk messing up your production feeds. I use foreman:

$ gem install foreman
$ foreman start

You can run background jobs with rake work_jobs and fetch feeds manually with rake fetch_feeds.

@dperson
Copy link
Copy Markdown
Contributor Author

dperson commented Feb 24, 2021

Yeah, I really don't know why Net::HTTP isn't reliable for queries... actually I just thought of something, I wonder if it doesn't follow redirects? If the formerly build into feedjira and HTTParty do... of course I just finished reverting back to HTTParty. How about we merge it, and I'll work at getting Net::HTTP to follow redirects?

@dperson
Copy link
Copy Markdown
Contributor Author

dperson commented Feb 24, 2021

Yeah, Net::HTTP doesn't follow redirects automatically, you have to check the status and manually follow them, yuck.

@mockdeep
Copy link
Copy Markdown
Collaborator

Ah, wow. Great sleuthing! So there is one other core ruby alternative, which is OpenURI. It appears that that properly follows redirects:

require "open-uri"

URI("http://www.quotationspage.com/data/qotd.rss").read
# => "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\r\n<?xml-stylesheet type=\"text/xsl\" ...

So it looks like we could use that instead. But I'll leave it up to you. HTTParty seems like it's probably more robust.

@dperson
Copy link
Copy Markdown
Contributor Author

dperson commented Feb 25, 2021

If it's okay with you, I'd like to merge the HTTParty version, and I'll work on redoing it with OpenURI later. That way if it is broken somehow, we can roll back to the version upstream recommends / tests.

I do like the idea of fewer external dependencies as well, so think it makes sense to move towards that goal.

@mockdeep mockdeep self-assigned this Feb 25, 2021
@mockdeep
Copy link
Copy Markdown
Collaborator

Sounds good to me!

@mockdeep mockdeep merged commit 7b4935b into stringer-rss:master Feb 25, 2021
@dperson dperson deleted the update-feedjira branch February 26, 2021 04:27
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.

2 participants