-
Notifications
You must be signed in to change notification settings - Fork 499
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 #3056 & #3076: Create a BUILD.bazel file for the logging package in the utility module #3079
Fix #3056 & #3076: Create a BUILD.bazel file for the logging package in the utility module #3079
Conversation
@fsharpasharp can you take a first pass on this & assign it back after it looks good to you? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! This was a lot of libraries. Main thing here is to create another BUILD.bazel file for the firebase specific libraries.
srcs = [ | ||
"LogLevel.kt", | ||
], | ||
visibility = ["//:oppia_api_visibility"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to omit the visibility field and make this library private? It seems like it is only used within the package itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I'll do it private
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@fsharpasharp I'm getting this error can you help me? |
I'm trying to push to origin |
Try pulling the latest develop branch first. |
I got this error after merging the latest changes into my local branch. I'm using GitHub oath tokens is it creating this issue @fsharpasharp ? |
Hm, could you check if there's anything on here that could help: https://stackoverflow.com/questions/64059610/how-to-resolve-refusing-to-allow-an-oauth-app-to-create-or-update-workflow-on I'm not familiar with the issue. |
@fsharpasharp if I run bazel tests locally it passes for both testing and data but here it fails. |
utility/src/main/java/org/oppia/android/util/logging/firebase/BUILD.bazel
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jonathanalvares9009. This LGTM.
The Bazel failure is unrelated to this PR, so we can ignore it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks
@fsharpasharp can you review my PR? |
Thanks @jonathanalvares9009 ! |
@BenHenning could you merge the PR? |
@JonathanAlvares FYI there are a couple of failing checks (see the Robolectric unit tests). |
Hey @jonathanalvares9009. Could you try updating the PR with the latest develop branch so we can look to get this merged? Also let me know if you're interested in doing more Bazel stuff as we look towards /domain and /app. |
@fsharpasharp yes I would like to work on Bazel issues |
Assigning @fsharpasharp to confirm and merge the PR if it's good to go. |
Looks good to me @anandwana001. I don't think I'm able to merge though. |
Looks like few tests are failing, should I merge PR with these failure? |
Yes, I think we can merge the PR with the failure. |
Created a BUILD.bazel file for the logging package in the utility module.
Fixes #3056 and #3076
Explanation
Checklist