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

Fetch released migrations via the GitHub API #14

Merged

Conversation

Fryguy
Copy link
Member

@Fryguy Fryguy commented Jun 21, 2017

This reverts the released migrations to using the GitHub API as was done
in 9fba1a4. The concern for the GitHub
rate limits in 9568c7a is no longer a
problem now that migrations are in their own repo which should have much
less churn, unlike the main manageiq repo where the migrations would run
on every PR.

Fixes #6

@carbonin Please review.

@Fryguy Fryguy added the bug label Jun 21, 2017
@chessbyte chessbyte requested a review from carbonin June 21, 2017 02:56
@chessbyte chessbyte self-assigned this Jun 21, 2017
@carbonin
Copy link
Member

Looks good except for what looks like a missing require.

This reverts the released migrations to using the GitHub API as was done
in 9fba1a4.  The concern for the GitHub
rate limits in 9568c7a is no longer a
problem now that migrations are in their own repo which should have much
less churn, unlike the main manageiq repo where the migrations would run
on *every* PR.

Fixes ManageIQ#6
@Fryguy Fryguy force-pushed the fetch_released_migrations_via_github_api branch from 08c8d52 to bd5ce03 Compare June 22, 2017 17:32
@miq-bot
Copy link
Member

miq-bot commented Jun 22, 2017

Checked commit Fryguy@bd5ce03 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks fine. ⭐

@chessbyte
Copy link
Member

@Fryguy Travis Failure for Ruby 2.4

@Fryguy
Copy link
Member Author

Fryguy commented Jun 23, 2017

I don't understand it...I run Ruby 2.4 locally and it works just fine. It can't be something GitHub related or both versions would fail. @carbonin Maybe the sporadic failures in the past weren't because of rate limiting?

@carbonin
Copy link
Member

Could be ... I also tried locally with 2.4 and didn't have any problems.

Do you think it would pass if we re-run it? (not that this would be a good thing necessarily).

@Fryguy
Copy link
Member Author

Fryguy commented Jun 23, 2017

Do you think it would pass if we re-run it

Just kicked it...let's see

@carbonin
Copy link
Member

😟

Copy link
Member

@carbonin carbonin left a comment

Choose a reason for hiding this comment

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

I'm going to go ahead and approve this. It looks like it should work, but we may end up dealing with sporadic failures it seems ...

@Fryguy
Copy link
Member Author

Fryguy commented Jun 23, 2017

aaaand it's green ☹️ (👈 never thought I'd be sad about something going green 😕 )

@Fryguy
Copy link
Member Author

Fryguy commented Jun 23, 2017

OK, let's merge and deal with consequences if it's truly sporadic.

@chessbyte Please merge. What we have here is better than not doing it, as not doing it means we leave devs with "broken" repos.

@chessbyte chessbyte merged commit cad19b8 into ManageIQ:master Jun 23, 2017
@chessbyte chessbyte added this to the Sprint 64 Ending Jul 3, 2017 milestone Jun 23, 2017
@Fryguy Fryguy deleted the fetch_released_migrations_via_github_api branch September 26, 2017 17:54
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.

4 participants