Skip to content
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

[Asset Inventory][Azure] Add test cases and installation script #2498

Merged
merged 19 commits into from
Sep 20, 2024

Conversation

kubasobon
Copy link
Member

@kubasobon kubasobon commented Sep 11, 2024

Summary of your changes

Add integration test suite and installation scripts for Azure Asset Inventory.

Screenshot/Data

Screenshot 2024-09-19 at 10 53 51 Screenshot 2024-09-19 at 10 54 48

Related Issues

Closes https://github.com/elastic/security-team/issues/10320
Requires elastic/integrations#11125 & elastic/integrations#11167 to be merged first

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • I have added the necessary README/documentation (if appropriate)

@kubasobon kubasobon self-assigned this Sep 11, 2024
Copy link

mergify bot commented Sep 11, 2024

This pull request does not have a backport label. Could you fix it @kubasobon? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v./d./d./d is the label to automatically backport to the 8./d branch. /d is the digit
    NOTE: backport-v8.x has been added to help with the transition to the new branch 8.x.

Copy link

mergify bot commented Sep 12, 2024

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b azure-asset-inventory-it upstream/azure-asset-inventory-it
git merge upstream/main
git push upstream azure-asset-inventory-it

@kubasobon kubasobon force-pushed the azure-asset-inventory-it branch 2 times, most recently from 3e8dc33 to 803411a Compare September 13, 2024 13:08
Copy link

mergify bot commented Sep 13, 2024

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b azure-asset-inventory-it upstream/azure-asset-inventory-it
git merge upstream/main
git push upstream azure-asset-inventory-it

@kubasobon kubasobon marked this pull request as ready for review September 19, 2024 08:57
@kubasobon kubasobon requested a review from a team as a code owner September 19, 2024 08:57
Copy link
Member

@romulets romulets left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me. Just some small comments. Maybe it's good that @gurevichdmitry have a look even if you merge today

run: poetry run pytest -k "asset_inventory_azure" --alluredir=./allure/results/ --clean-alluredir

- name: Upload test results
if: ${{ always() }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have an if always?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So that it uploads results even if the previous step fails. Otherwise it skips all following steps, including this one.

It's a copy of regular Azure action.

@@ -67,6 +67,11 @@ jobs:
shell: bash
run: terraform fmt -check -recursive

- name: Show git diff on failure
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was missing this - now it shows you what the diff is if pre-commit results in changes. I was having trouble replicating linter output on my machine.

current_version=package_version,
required_version=PKG_DEFAULT_VERSION,
):
logger.warning(f"{INTEGRATION_NAME} is not supported in version {package_version}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are sys.exiting it, feels like a error deserving log 😬

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a copy of what other scripts are doing, but sure, I can bump log level.

@kubasobon
Copy link
Member Author

Ok. Merging. I can change log levels in a separate PR if important.

@kubasobon kubasobon merged commit 33b57c7 into main Sep 20, 2024
11 checks passed
@kubasobon kubasobon deleted the azure-asset-inventory-it branch September 20, 2024 15:13
opauloh pushed a commit to opauloh/cloudbeat that referenced this pull request Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants