-
Notifications
You must be signed in to change notification settings - Fork 28
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
Send authors to API #208
Send authors to API #208
Conversation
8837064
to
d9ef4e2
Compare
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.
Looks Neat To me 🚀
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 left a few ideas on what we could improve.
096015a
to
53f4c6b
Compare
@@ -17,6 +17,8 @@ def queue(args) | |||
:node_index => args.fetch(:node_index), | |||
:node_build_id => KnapsackPro::Config::Env.ci_node_build_id, | |||
:user_seat => KnapsackPro::Config::Env.masked_user_seat, | |||
:build_author => KnapsackPro::RepositoryAdapters::GitAdapter.new.build_author, | |||
:commit_authors => KnapsackPro::RepositoryAdapters::GitAdapter.new.commit_authors, |
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.
We could do a performance improvement here and move the lines down below. I completely forgot about it. Just noticed it now.
We can send build author and commit authors only in the request that supposed to initialize a new queue. Thanks to that we don't have to transfer unnecessary data over the network for each Queue API request that is consuming tests from the Queue. Similarly like we do for test files.
Additional benefit of that approach is that we don't have to call git command on each request. I guess this could cause a small delay in running tests if you call git many times on each CI node for each request.
if request_hash[:can_initialize_queue] && !request_hash[:attempt_connect_to_queue]
request_hash.merge!({
:test_files => args.fetch(:test_files),
:build_author => KnapsackPro::RepositoryAdapters::GitAdapter.new.build_author,
:commit_authors => KnapsackPro::RepositoryAdapters::GitAdapter.new.commit_authors
})
end
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.
private | ||
|
||
def git_commit_authors | ||
`git fetch --shallow-since "1 month ago" >/dev/null 2>&1 && git shortlog --summary --email --since "one month ago"` |
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.
If you run knapsack pro locally on your machine, you call --shallow-since
command and when I want to use git blame
, I only see commits from the last month.
I undo changes by using the following command:
git fetch --unshallow || git fetch --all
I guess someone might not like that we mess up with their local repository when they try to run Knapsack Pro locally.
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 for the report.
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.
For reference: The above issue will be solved when we run --shallow-since
only in the context of the CI. This PR solves it:
Send masked authors to API:
build_author
is the name and email of the author for the last commitcommit_authors
is author (as above) + number of commits obtained by grouping all the commits of the last monthA
git fetch
is performed to make sure the last months of commits are available in case the repository is performing shallow clones for performance reasons.Notice that this solution respects gitmailmap, which could be useful to customers/us.
See also https://github.com/KnapsackPro/knapsack-pro-api/pull/726