-
Notifications
You must be signed in to change notification settings - Fork 0
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
OAuth2 Github client #7
Conversation
Welcome to Codecov 🎉Once merged to your default branch, Codecov will compare your coverage reports and display the results in this comment. Thanks for integrating Codecov - We've got you covered ☂️ |
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.
@hizmailovich take a look, at my comments
src/main/java/git/tracehub/pmo/agents/github/InviteCollaborator.java
Outdated
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.
@hizmailovich just a few small comments
src/main/java/git/tracehub/pmo/agents/github/InviteCollaborator.java
Outdated
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.
@hizmailovich take a look at my comments, please
* SOFTWARE. | ||
*/ | ||
|
||
package git.tracehub.pmo.agents; |
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.
I don't think this logic is related to agents
, this name its very abstract, names
like github
, gitlab
, bitbucket
will be more suitable here.
see the point?
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.
This logic related to all agents, that's why RepoPath
is located here, but this package also includes such packages as github
, gitlab
, and bitbucket
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.
@hizmailovich
My comment was related to problem that these objects are not agents
, here you are making some actions with operators/target platforms like GitHub, GitLab and so on.
So I propose to stay with package naming that includes just a platform name. Its a good idea to group so-called agents, but agents are not the best name here, I believe.
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.
Renamed to platforms
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.
@hizmailovich one comment on real-export.json
imports/realm-export.json
Outdated
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.
Do we really need this?
I think if it some secret
that must be present at application runtime, lets move it from this repo to secrets
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.
This file just contains of the information about realm without any sensitive data, moreover we need this file in such location to use it in docker-compose
what do you think?
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.
@hizmailovich place it inside root then
@hizmailovich sorry for double review here. |
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.
@hizmailovich commit look good, answered on your questions, see above
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.
@hizmailovich good one
@rultor try to merge |
@rultor merge |
@h1alexbel Thanks for your request; @hizmailovich please confirm this. |
Closes #9
PR-Codex overview
This PR focuses on adding new files and making changes related to database configuration, licensing, and code comments.
Detailed summary
ANALYZE my_table
toanalyze.sql
db/changelog/2024
todb.changelog-master.yaml
application-pgit.yaml
andapplication.yaml
LICENSE.txt
andpackage-info.java
files with.git
extensionprojects.sql
application.yaml
docker-compose.yml