-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add job for ESCU Tests #358
base: develop
Are you sure you want to change the base?
Conversation
1981dfe
to
4a71719
Compare
72f8f82
to
78ef048
Compare
4de44b9
to
993aede
Compare
0698fba
to
bc87701
Compare
- name: Install Python Dependencies and ContentCTL | ||
run: | | ||
python -m pip install --upgrade pip | ||
pip install contentctl |
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.
Wondering if we should discuss with Security Content team to have releases matching somehow the repo, as downloading develop and newest tool may be problematic at some 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.
Yeah, we can
I asked this in our ESCU channel - https://splunk.slack.com/archives/C088U396U11/p1739876980416399?thread_ts=1739268536.325259&cid=C088U396U11
But Eric mentioned Unfortunately we don’t have a specific version of contentctl called out for building security_content
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.
Yes yes, they do not have, but can have :)
# Load the YAML content | ||
with open("security_content/contentctl.yml", "r") as file: | ||
data = yaml.safe_load(file) | ||
for app in data["apps"]: | ||
if app['appid'] == APP_ID or GITHUB_REPOSITORY in app['hardcoded_path'] or app["title"] == APP_LABEL: | ||
app['hardcoded_path'] = "${{ env.TA_BUILD_PATH }}" | ||
elif app['appid'] == "PALO_ALTO_NETWORKS_ADD_ON_FOR_SPLUNK" and APP_ID == "Splunk_TA_paloalto_networks": | ||
app['hardcoded_path'] = "${{ env.TA_BUILD_PATH }}" |
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.
# Load the YAML content | |
with open("security_content/contentctl.yml", "r") as file: | |
data = yaml.safe_load(file) | |
for app in data["apps"]: | |
if app['appid'] == APP_ID or GITHUB_REPOSITORY in app['hardcoded_path'] or app["title"] == APP_LABEL: | |
app['hardcoded_path'] = "${{ env.TA_BUILD_PATH }}" | |
elif app['appid'] == "PALO_ALTO_NETWORKS_ADD_ON_FOR_SPLUNK" and APP_ID == "Splunk_TA_paloalto_networks": | |
app['hardcoded_path'] = "${{ env.TA_BUILD_PATH }}" | |
# Load the YAML content | |
with open("security_content/contentctl.yml", "r") as file: | |
data = yaml.safe_load(file) | |
found = False | |
for app in data["apps"]: | |
if app['appid'] == APP_ID or GITHUB_REPOSITORY in app['hardcoded_path'] or APP_LABEL in app['title']: | |
app['hardcoded_path'] = "${{ env.TA_BUILD_PATH }}" | |
found = True | |
if not found: | |
exit(127) |
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.
Don't we need this condition as well?
elif app['appid'] == "PALO_ALTO_NETWORKS_ADD_ON_FOR_SPLUNK" and APP_ID == "Splunk_TA_paloalto_networks":
Because the name in contentctl is I guess of the vendor TA and we have Splunk TA for palo alto.
So, unless the contentctl is updated with the proper name, we will need this.
name: escu_test_summary_results | ||
path: | | ||
security_content/test_results/summary.yml | ||
security_content/dist/DA-ESS-ContentUpdate-latest.tar.gz |
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.
Dow we need ths additonal file?
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 think we can remove that.
echo "contentctl test --container-settings.num-containers 8 --post-test-behavior never_pause --container-settings.no-leave-running mode:selected --mode.files ${{ steps.filter-detection-files.outputs.DETECTION_FILES }}" | ||
contentctl test --container-settings.num-containers 8 --post-test-behavior never_pause --container-settings.no-leave-running mode:selected --mode.files ${{ steps.filter-detection-files.outputs.DETECTION_FILES }} |
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.
Is failure of test making build red? - it should.
Please also rebase first on newest develop @spanchal-crest |
- name: Run Python Script | ||
id: filter-detection-files | ||
shell: python | ||
run: | |
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.
Can we move it out to a separate file / script?
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.
Can I know what would be the appropriate place to put that file?
echo "contentctl test --container-settings.num-containers 8 --post-test-behavior never_pause --container-settings.no-leave-running mode:selected --mode.files ${{ steps.filter-detection-files.outputs.DETECTION_FILES }}" | ||
contentctl test --container-settings.num-containers 8 --post-test-behavior never_pause --container-settings.no-leave-running mode:selected --mode.files ${{ steps.filter-detection-files.outputs.DETECTION_FILES }} |
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.
How did you solve the performance aspect of running for big add-ons?
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 have run the detections using a larger runner and it is very fast.
ESCU Tests in Windows TA using large runner (32 cores)
Using 4 containers - https://github.com/splunk/splunk-add-on-for-microsoft-windows/actions/runs/13492512521/job/37693050535 took ~52min
Using 8 containers - https://github.com/splunk/splunk-add-on-for-microsoft-windows/actions/runs/13492512521/job/37701127717 took ~35min
Using 12 containers - https://github.com/splunk/splunk-add-on-for-microsoft-windows/actions/runs/13492512521/job/37703683659 took ~35min
We have also tried other approach of splitting the tests into multiple jobs.
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 have also tried other approach of splitting the tests into multiple jobs.
Can you please share the results for it?
Can I run the tests locally? What would be the performance of it? I expect folks to run tests (if possible) locally before pushing code to GitHub.
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.
@mgazda-splunk we run it completely in GitHub while other types of tests we do run in AWS, right? Did you consider running it in AWS instead? This PR brings testing logic into the reusable workflow - we use https://github.com/splunk/ta-automation-k8s-manifests/blob/main/test-addon.sh for testing logic.
859cc31
to
9f47022
Compare
9f47022
to
c9f35c3
Compare
c9f35c3
to
3cb3e73
Compare
No description provided.