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

Adapt legacy code to new version of application tests infrastructure #24

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

Conversation

nshyrei
Copy link
Contributor

@nshyrei nshyrei commented Jan 10, 2025

This PR alters the legacy behavior of container-converter to fit with the new version of app-test-infra (https://github.com/fortanix/app-test-infra) when running application tests. In summary the following was done:

  1. Container converter now prints json serialized conversion response into the stdout. Previously the response was serialized as a simple string and it would break the code in: https://github.com/fortanix/app-test-infra/blob/5abda4112fb64559eb4e0064cf78dc7e4a29cb0a/python/test_app.py#L1005C1-L1008C1
  2. Container converter now prints a special key phrase when printing the response to stdout. This phrase is used to find the response by app-test infra code in: https://github.com/fortanix/app-test-infra/blob/5abda4112fb64559eb4e0064cf78dc7e4a29cb0a/python/test_app.py#L1383
  3. The unfinished legacy debug image logic is completely removed from container converter. Previously it was part of the idea for converter to create a special image with debug tooling and a distinct suffix when debug flag is set to true. This was never realized into a full feature and now breaks the application tests infrastructure code because images with debug prefixes are not supported there. As this feature was never properly implemented and was never needed that much I believe it's best to remove it entirely.
  4. Changed path of DOCKER-AWS-HELPER to be consistent with tool installation path on the executor machine.

@nshyrei nshyrei added the enhancement New feature or request label Jan 10, 2025
@nshyrei nshyrei self-assigned this Jan 10, 2025
let response_serialized = serde_json::to_string(&response)
.map_err(|err| format!("Failed serializing conversion request. {:?}", err))?;

println!("Successful nitro conversion: {:?}", response_serialized);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aditijannu
Copy link
Collaborator

Which jira ticket requires these changes?
app-test-infra code is shared between zircon and salmiac. These changes should ideally be tested in both projects. Otherwise it is likely going to break one or both projects.

Instead of removing the debug image logic completely, shouldn't we be fixing it in another ticket?

@nshyrei
Copy link
Contributor Author

nshyrei commented Jan 14, 2025

Which jira ticket requires these changes? app-test-infra code is shared between zircon and salmiac. These changes should ideally be tested in both projects. Otherwise it is likely going to break one or both projects.

Instead of removing the debug image logic completely, shouldn't we be fixing it in another ticket?

@aditijannu The changes are related to a jira story where I move application tests CI to jenkins (RTE-215). I don't understand how these changes will break ZIrcon if I only touch Salmiac-related code here. About the debug image - it also prevents me from running the app-tests currently as the suffix addition logic prevents the image to be found by app-test code. Also nobody has ever used debug images as this feature wasn't really complete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants