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

chore: refactor actions code #2276

Closed
wants to merge 24 commits into from
Closed

Conversation

Racer159
Copy link
Contributor

@Racer159 Racer159 commented Jan 31, 2024

Description

This PR introduces conditionals to Zarf actions to help them function in a cross-platform manner.

Related Issue

Relates to #2253
Relates to #2273

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Other (security config, docs update, etc)

Checklist before merging

Copy link

netlify bot commented Jan 31, 2024

Deploy Preview for zarf-docs canceled.

Name Link
🔨 Latest commit cfb6efd
🔍 Latest deploy log https://app.netlify.com/sites/zarf-docs/deploys/65eb8c6cf352b90008022ecd

@Racer159 Racer159 added needs-adr PR Label - Architectural Decision Records required to merge needs-docs PR Label - Docs required to merge needs-tests PR Label - Tests required to merge labels Jan 31, 2024
@Racer159 Racer159 requested a review from a team as a code owner February 1, 2024 02:31
@Racer159 Racer159 marked this pull request as draft February 1, 2024 02:31
Copy link
Contributor

@Noxsios Noxsios left a comment

Choose a reason for hiding this comment

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

first runthrough, didnt view every file (54/70), but wanted to review this PR in chunks anyways

Comment on lines -192 to 196
func validateActionset(actions types.ZarfComponentActionSet) (bool, error) {
func validateActionset(actionSet actions.ActionSet) (bool, error) {
containsVariables := false

Copy link
Contributor

Choose a reason for hiding this comment

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

imo this could now be changed to a receiver on actions.ActionSet. Reason this was a function that took in an ActionSet was because it was from types and you cannot put receivers on types from other packages.

Copy link
Contributor

Choose a reason for hiding this comment

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

if we move actions to the common pkg, this code will have to be brought over as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to only pull actions.Validate() over - this logic is partly Zarf specific since it is looking for variables which only matters because Zarf has actions that don't support them (the rest of the logic is just simply looping through actions in the actionset)

src/internal/packager/helm/post-render.go Outdated Show resolved Hide resolved
src/internal/packager/template/template.go Outdated Show resolved Hide resolved
src/pkg/actions/actions.go Outdated Show resolved Hide resolved
src/pkg/actions/types.go Outdated Show resolved Hide resolved
src/pkg/actions/types.go Outdated Show resolved Hide resolved
Comment on lines 22 to 28
// MakeTempDir creates a temp directory with the zarf- prefix.
func MakeTempDir(basePath string) (string, error) {
if basePath != "" {
if err := CreateDirectory(basePath, 0700); err != nil {
if err := helpers.CreateDirectory(basePath, 0700); err != nil {
return "", err
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

MakeTempDir can prob be brought over to helpers (unless the internal debug is what made you keep it here)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was also the temp path prefix - IMO it is relatively Zarf specific - os.MkdirTemp is there for folks that want an OOB experience.

Errorf(err error, format string, a ...any)
Successf(format string, a ...any)
Updatef(format string, a ...any)
Write(p []byte) (n int, err error)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you thinking this is a spinner or progressbar so someone may want to write progress? Would you ever be able to measure how much of an action is really done though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spinner, the write method on spinner works differently from progressbar in that it takes a stdOut/stdErr stream

Racer159 pushed a commit that referenced this pull request Feb 19, 2024
## Description

#2180 introduced a bug and
this PR removes the cause of the bug.

Actions conditionals are being added to Zarf in
#2276 to allow these sort of
checks to account for various use cases in a more clean way.

Also reopened #1824

## Related Issue

Relates to #2273

## Type of change

- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Other (security config, docs update, etc)

## Checklist before merging

- [x] Test, docs, adr added or updated as needed
- [x] [Contributor Guide
Steps](https://github.com/defenseunicorns/zarf/blob/main/CONTRIBUTING.md#developer-workflow)
followed
@Racer159 Racer159 changed the title feat: introduce actions conditionals feat: refactor actions code Feb 20, 2024
@Racer159 Racer159 changed the title feat: refactor actions code chore: refactor actions code Feb 20, 2024
@Racer159 Racer159 removed needs-adr PR Label - Architectural Decision Records required to merge needs-docs PR Label - Docs required to merge labels Feb 20, 2024
@lucasrod16
Copy link
Contributor

After discussing with @Racer159, this PR will not end up being merged, so closing this PR. The 2273-storage-class-check-edge-cases branch should not be deleted though.

@lucasrod16 lucasrod16 closed this May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-tests PR Label - Tests required to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants