Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Feature/rewarded user #31

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

Open
berinhardt wants to merge 10 commits into firebase:main
base: main
Choose a base branch
Loading
from berinhardt:Feature/Rewarded_UserID

Conversation

Copy link

@berinhardt berinhardt commented Nov 15, 2019

No description provided.

Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

i️ Googlers: Go here for more info.

Copy link
Author

@googlebot I signed it!

Copy link

CLAs look good, thanks!

i️ Googlers: Go here for more info.

Copy link
Contributor

Hi @berinhardt,

Thanks for sending the pull request! Unfortunately we won't be able to accept it currently, as we are still working on setting up the test infrastructure for the repository, as we need to support a variety of build environments, and therefore don't have a great way to automatically verify it. We'll be sure to reach out when we are in a position to bring this in.

Note that because this is a bigger change, which is adding new functions to the public headers (and thus changing the API surface), it might take a bit longer to verify. When the repo is set up to take in pull requests, we will be sure to describe the workflow in the CONTRIBUTING.md.

Again, thanks for the interest, great to see people getting involved!

@DellaBitta DellaBitta deleted the branch firebase:main February 24, 2021 18:46
@DellaBitta DellaBitta changed the base branch from master to main February 25, 2021 14:30
Copy link
Contributor

jonsimantov commented Mar 29, 2021
edited
Loading

Hi @berinhardt,

Sorry for the extremely long delay in getting a look at this PR. We finally have the infrastructure in place to take code submissions. If you're interested in getting this PR in, please merge the latest changes from main into it, and we can take a look.

Also note that there is now an AdMob integration test in admob/integration_test/src/integration_test.cc -- if you could add a test for this new functionality, that would be helpful.

Finally, one more note - to be able to use our testing infrastructure, this change should be made in a feature branch within this Git repo. You should have permissions to create a feature branch in the firebase/firebase-cpp-sdk project, and merge the changes from this PR into that branch.

Thanks!

Copy link
Author

Will do.

Copy link
Author

Finally, one more note - to be able to use our testing infrastructure, this change should be made in a feature branch within this Git repo. You should have permissions to create a feature branch in the firebase/firebase-cpp-sdk project, and merge the changes from this PR into that branch.

Are you sure? I'm not being able to create that branch.

Copy link
Contributor

Finally, one more note - to be able to use our testing infrastructure, this change should be made in a feature branch within this Git repo. You should have permissions to create a feature branch in the firebase/firebase-cpp-sdk project, and merge the changes from this PR into that branch.

Are you sure? I'm not being able to create that branch.

Interesting. What's the command that you're running and what's the error? Thanks!

Copy link
Author

just tried to push a new branch named feature/RewardedUserId and returned error 403. What I'm not understanding?

Copy link
Contributor

DellaBitta commented Mar 31, 2021
edited
Loading

I don't know but we'll work it out!

How are you attempting to create a new branch? I'm not familiar with a method in Github that would return HTTP request error codes -- learning more of the details about what you're attempting would be helpful.

I'm personally using the git command line tools and my flow for creating a new branch would look something like this (assuming I've already git cloned the repo):

git checkout main
git pull
git branch feature/abc123
git checkout feature/abc123

... make some changes to a file ...

git add <file>
git commit -m "I changed a file"
git push --set-upstream origin feature/abc

Thanks.

Copy link
Author

Exactly like that, but when I try to push to the origin, it tries to push to https://github.com/firebase/firebase-cpp-sdk.git, and says 403

I'll try adding an ssh origin in order to see if I can get another error

Copy link
Contributor

I was just able to reproduce this with a new account and using http as well. I'm looking into it, but it would be great to know if ssh works.

Copy link
Author

with ssh doesn't work either

ERROR: Permission to firebase/firebase-cpp-sdk.git denied to berinhardt.

DellaBitta reacted with eyes emoji

Copy link
Contributor

Hi @berinhardt,

Thanks for bearing with us. We're just getting used to how user submissions work so I'm sorry for the confusion.

It turns out that the organization does not enable writes (pushes) by external contributors. The process of creating a PR from a fork is the correct manner to contribute, as you've done.

I can also see that the tests are running. If there's a problem that tests are failing due to the origin being a fork, then that's something that we'll have to work on. A potential solution is for one of the admins to create a branch, merge your changes into it, and then we can re-execute the tests. We'll see how this next text execution pans out.

Copy link
Author

all tests done

@DellaBitta DellaBitta added the tests-requested: quick Trigger a quick set of integration tests. label Apr 12, 2021
Copy link
Author

berinhardt commented Apr 19, 2021
edited
Loading

Something seems to be wrong with your integration tests

Error: Resource not accessible by integration?

@jonsimantov jonsimantov removed the tests-requested: quick Trigger a quick set of integration tests. label May 3, 2021
Copy link
Contributor

We've fixed our testing for contributors - if you merge main into your branch, I'll be able to trigger the full integration test suite on your PR manually.

Copy link
Author

Sorry for the delay

Copy link
Author

what else is needed for this?

Copy link
Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /