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

Refactor init_containers building logic in TemplateMixin #1157

Merged
merged 2 commits into from
Aug 19, 2024

Conversation

shyoon-devops
Copy link
Contributor

@shyoon-devops shyoon-devops commented Aug 19, 2024

Pull Request Checklist

PR Description

When using Hera to generate an Argo Workflow, I encountered an issue with the env structure in initContainers. Specifically, the env variable references for secrets are not being properly constructed. Instead of the correct structure with valueFrom, the generated YAML output directly assigns secret_name and secret_key, which is not valid in Argo Workflows.

This issue occurs because Hera doesn't handle this internally within its template constructors. Instead, it uses the initContainers as they are provided here. To address this, I created the _build_init_containers method in the TemplateMixin class.

    def _build_init_containers(self) -> Optional[List[ModelUserContainer]]:
        """Builds the `init_containers` field and optionally returns a list of `UserContainer`."""
        if self.init_containers is None:
            return None

        return [i.build() if isinstance(i, UserContainer) else i for i in self.init_containers]

This method ensures that the init_containers field is correctly constructed, and if any of the containers are instances of UserContainer, they are properly built before being returned.

@elliotgunton elliotgunton changed the title Refactor init_containers building logic in TemplateMixin (#1155) Refactor init_containers building logic in TemplateMixin Aug 19, 2024
elliotgunton added a commit that referenced this pull request Aug 19, 2024
See argoproj/argo-workflows#13429

Fixes Python for upstream files so `make regenerate-test-data` works as
expected (to unblock #1157)

Signed-off-by: Elliot Gunton <elliotgunton@gmail.com>
@samj1912 samj1912 added semver:patch A change requiring a patch version bump type:bug A general bug labels Aug 19, 2024
Copy link

codecov bot commented Aug 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.4%. Comparing base (d553546) to head (49e3d57).
Report is 124 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1157     +/-   ##
=======================================
- Coverage   81.7%   81.4%   -0.3%     
=======================================
  Files         54      60      +6     
  Lines       4208    4722    +514     
  Branches     889    1001    +112     
=======================================
+ Hits        3439    3846    +407     
- Misses       574     646     +72     
- Partials     195     230     +35     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@elliotgunton elliotgunton left a comment

Choose a reason for hiding this comment

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

Thank you! 🚀

@elliotgunton elliotgunton merged commit 276c5e4 into argoproj-labs:main Aug 19, 2024
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:patch A change requiring a patch version bump type:bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue: Incorrect env Structure in initContainers Generated by Hera
3 participants