Skip to content
This repository has been archived by the owner on Sep 26, 2024. It is now read-only.

Add support for commit_hash on test #111

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

matbalc
Copy link

@matbalc matbalc commented Sep 29, 2023

Add support for commit_hash, branch_tag and build_id on tests.
DefectDojo models supports these values on test level so it would be nice ddimport to allow to set these values. I have not added validation as exists on Engagement level, to threat them as optional

@StefanFl
Copy link
Contributor

StefanFl commented Oct 7, 2023

Hi @matbalc, thank you very much for your contribution. I would propose 2 changes, to make it even better:

  • We should introduce 3 new environment variables: DD_TEST_BUILD_ID, DD_TEST_COMMIT_HASH and DD_BRANCH_TAG. Then a user can decide if they want to set the attributes in an engagement or in a test.
  • The attributes should be set with an update routine after the import, like it is done for the engagement. Currently they can only be set for a new test, but not be updated in subsequent re-imports.

@matbalc
Copy link
Author

matbalc commented Oct 7, 2023

Hi @matbalc, thank you very much for your contribution. I would propose 2 changes, to make it even better:

  • We should introduce 3 new environment variables: DD_TEST_BUILD_ID, DD_TEST_COMMIT_HASH and DD_BRANCH_TAG. Then a user can decide if they want to set the attributes in an engagement or in a test.
  • The attributes should be set with an update routine after the import, like it is done for the engagement. Currently they can only be set for a new test, but not be updated in subsequent re-imports.

@StefanFl good catch regarding capability to provide hash while re-uploading the test. I will adjust.
However, regarding introducing the new variables to track commit_hash for test separately from engagement is there a need for this? I can't think of scenario where I would want to set different commit hashes for engagement and for test. Could you help me to understand the need for it? Nonetheless, I don't think this is bad idea, will adjust. Just trying to think what is the actual added value in "real world scenario"

@StefanFl
Copy link
Contributor

StefanFl commented Oct 8, 2023

@matbalc You never know what users want to do. So in my experience it's better to be flexible and let users have the choice, if they want to set the attributes for the engagement or the tests.

@matbalc
Copy link
Author

matbalc commented Oct 17, 2023

@StefanFl
I have updated PR to reflect your suggested changes

@StefanFl
Copy link
Contributor

Hi @matbalc , thanks for the changes so far. The second part is still missing, so that the attributes can be changed in suqsequent imports, like for engagements.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants