[CIAPP-5371] validate git tags #15
Conversation
anmarchenko
changed the title
There was a problem hiding this comment.
I like the polymorphic approach that each extractor encapsulates vendor specific logic, but I expect there should be corresponding test for each of them, am i missing?
When I read through the changes, there are a lot of methods that are returning nil and performing nil check. This causes a lot of cognitive burden and I wonder if there is a better way to handle it.
| def git_commit_sha | ||
| end | ||
|
|
||
| def normalize_git!(tags) |
There was a problem hiding this comment.
Input tags conflict with the method tags, I would suggest extract this stateless function.
There was a problem hiding this comment.
maybe I need separate modules for normalization and expansion 🤔
There was a problem hiding this comment.
@TonyCTHsu I made this more readable using instance variable @tags and removing some nil checks (moved the code that removes nil tags after the normalization)
| fields = output.split("\t").each(&:strip!) | ||
|
|
||
| @commit_users = { | ||
| author_name: fields[0], | ||
| author_email: fields[1], | ||
| # Because we can't get a reliable UTC time from all recent versions of git | ||
| # We have to rely on converting the date to UTC ourselves. | ||
| author_date: Time.at(fields[2].to_i).utc.to_datetime.iso8601, | ||
| committer_name: fields[3], | ||
| committer_email: fields[4], | ||
| # Because we can't get a reliable UTC time from all recent versions of git | ||
| # We have to rely on converting the date to UTC ourselves. | ||
| committer_date: Time.at(fields[5].to_i).utc.to_datetime.iso8601 | ||
| } |
There was a problem hiding this comment.
Perhaps consider extracting an CommitUser object instead of a hash?
It is less readable when each field is depending on the index of the array for variable fields.
Consider
name, email, ... = output.split("\t").each(&:strip!)
There was a problem hiding this comment.
thanks for this, will explore
There was a problem hiding this comment.
@TonyCTHsu I refactored this code to use GitUser concept but I am not sure if it is actually better than hash... wdyt about it now?
|
@TonyCTHsu I will add tests for each separate class as you suggested: they are all thoroughly tested by As for the nil checks: we are dealing with environment so everything can be nil at all times as we don't control what comes from the external environment. Will try to find a better way to define checks. |
|
@TonyCTHsu @GustavoCaso I added more specs, moved fixtures around and refactored some bits of code |
|
@anmarchenko There was something that wasn't clicking with me with the organization of the code.
It felt like a smell to me. I open a DRAFT PR to show you want I mean #18 Let me know what you think |
|
@GustavoCaso I like your cleanup, especially the fact that it draws a clear line between Extractor and Provider concepts. I will incorporate this change in this PR today |
show alternative code organization around provider and extractor
|
@GustavoCaso @TonyCTHsu now it is finally finally finally ready for review |
What does this PR do?
In cases when it is not possible to automatically extract git environment information for CI visibility we allow users to supply this info via environment variables, see docs here.
DD_GIT_REPOSITORY_URL and DD_GIT_COMMIT_SHA are required but currently not validated. When these values are not provided or SHA is not valid CI-APP backend silently drops these traces. End users get no feedback in these cases.
We add the following validations to these tags:
DD_GIT_REPOSITORY_URL must be present
DD_GIT_COMMIT_SHA must be present, must be 40 characters long must be a valid hex number
In case these values are incorrect we continue to send traces but we log an error message explaining the problem.
Motivation
Improve end user experience and prevent confusion when test spans are not shown in UI without any reason.
Additional Notes
This PR includes several refactorings:
How to test the change?
Tested according to CI spec in
environment_spec.rb