-
Notifications
You must be signed in to change notification settings - Fork 42
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
2718 audit project provisioning 2 #2818
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2818 +/- ##
==========================================
+ Coverage 91.37% 91.40% +0.03%
==========================================
Files 338 338
Lines 12052 12067 +15
==========================================
+ Hits 11012 11030 +18
+ Misses 1040 1037 -3 ☔ View full report in Codecov by Sentry. |
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 detailed explanation on the PR Rory. The shortcut link to the commit was helpful.
Left only one request for a higher level function on audit_workflows
🙏
project_changeset | ||
|> get_assoc(:workflows) | ||
|> Enum.reduce( | ||
Multi.new(), |
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.
Being already part of a transaction it wasn't necessary to be on a Multi. Nonetheless, I see little benefit from refactoring it to a reduce_while
for now.
classify_audit(workflow_changeset) | ||
|> case do | ||
{:no_action, _nil} -> | ||
Multi.new() | ||
|
||
{action, workflow_id} -> | ||
audit_workflow_multi(action, workflow_id, user_or_repo_connection) | ||
end | ||
|
||
Multi.append(multi, additional_multi) |
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 imagine it would be more readable replacing the case assignment with a function append_multi
or a smarter audit_workflow_multi
that does all the logic. Here on the high level would contain only the iteration.
data: %{id: workflow_id}, | ||
changes: changes | ||
}) | ||
when changes != %{} do |
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.
Got it, that's why you haven't used the Changeset.get_field
|
||
def provisioner_event(action, workflow_id, actor) do | ||
event( | ||
"#{past_tense(action)}_by_provisioner", |
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.
Neat!
Description
Audit when a project is inserted, updated or deleted via the provisioning api.
This PR includes changes to the tests to in preparation for the new code and this makes the PR a bit noisy. Working though it commit-by-commit will make it easier to follow. The actual code change is in this commit.
Closes #2718
Validation steps
/tmp
on your workstation.delete.json
insert.json
update.json
From the terminal execute an insertion:
Then, in your IEx session:
The
event
attribute oflatest_event
should be set toinserted_by_provisioner
.From the terminal execute an update:
Then, in your IEx session:
The
event
attribute oflatest_event
should be set toupdated_by_provisioner
.From the terminal execute a deletion:
Then, in your IEx session:
The
event
attribute oflatest_event
should be set todeleted_by_provisioner
.AI Usage
Please disclose how you've used AI in this work (it's cool, we just want to know!):
You can read more details in our Responsible AI Policy
Pre-submission checklist
:owner
,:admin
,:editor
,:viewer
)