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

Fix master build #147

Merged
merged 2 commits into from
Dec 30, 2020
Merged

Conversation

Sinetheta
Copy link
Contributor

@Sinetheta Sinetheta commented Dec 14, 2020

Hey all, I've included here 2 small fixes to solidus_graphql_api and a temporary workaround for the current canonical-rails problem.

The first fix is to specify a factory attribute which changed in Solidus master solidusio/solidus#3849

The second is to rescue a novel exception raised by newer versions of the gem graphql rmosolgo/graphql-ruby#2896

The store factory was updated in solidusio/solidus#3849

Although I see the "with_args" helper is being used elsewhere to
allow for asserting against dynamic payloads, the only reason
this value is changing on us is because of the unspoken dependency
on the attributes in a solidus factory. If Solidus is going to
embrace this kind of churn then we'd probably be best to set
all relevant attributes explicitly.
@kennyadsl
Copy link
Member

@Sinetheta Hey there! 👋

Thanks for the fixes, I think that the first one should be addressed now that a new version is cut.

@Sinetheta Sinetheta force-pushed the sinetheta/more-green branch from 2fbba30 to ef239ca Compare December 15, 2020 16:09
@Sinetheta
Copy link
Contributor Author

Hi @kennyadsl I've dropped the last commit.

@jarednorman
Copy link
Member

👋🏻 Hi @Sinetheta!

@Sinetheta
Copy link
Contributor Author

Hey Jared!

@kennyadsl that failure looks like it might be a Circle config issue

Error: you must supply a CC_TEST_REPORTER_ID ENV variable or pass it via the -r flag

Maybe the env is in the wrong "context" in the GUI?

@kennyadsl
Copy link
Member

@AlessioRocco can you help us here? Is the CI supposed to run correctly?

@kennyadsl
Copy link
Member

@Sinetheta CC_TEST_REPORTER_ID is a plain ENV variable (not a context one) and we are passing secrets to forks' PRs in CircleCI. Really not sure why this happens.

@kennyadsl
Copy link
Member

@Sinetheta did you created (by following) a CircleCI project for this repo in your own account? I think that by doing that CircleCI will use your own conf and variables so it doesn't pass the ones we defined in the main repo. Can you please check and confirm it's that? If yes, I think that removing your project (by unfollowing?) will fix the issue. LMK!

@Sinetheta Sinetheta force-pushed the sinetheta/more-green branch 2 times, most recently from a43a3b2 to 2ada973 Compare December 16, 2020 15:37
Here we are attempting to parse  arbitrary input as generated
object IDs. In version 1.10.8 there was a bug fix which  moved
some error handling code down deeper to the base 64 decoder.
In order to support both versions of the decoder we'll need
to rescue both types of exceptions.
@Sinetheta Sinetheta force-pushed the sinetheta/more-green branch from 2ada973 to 413a78c Compare December 16, 2020 15:38
@Sinetheta
Copy link
Contributor Author

Sorry @kennyadsl you're right I didn't notice that this PR was building in my fork of the project. FYI it wasn't enough to unfollow the project, there's now a "Stop Building" button in the Project Settings that I had to push.

Copy link
Contributor

@AlessioRocco AlessioRocco left a comment

Choose a reason for hiding this comment

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

LGTM 👍 . Thanks for your pull request.

@ChristianRimondi ChristianRimondi merged commit acb48dd into solidusio:master Dec 30, 2020
@Sinetheta Sinetheta deleted the sinetheta/more-green branch December 30, 2020 17:45
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.

5 participants