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

Implement readDirEntries method #254

Merged
merged 1 commit into from
Jul 27, 2022
Merged

Implement readDirEntries method #254

merged 1 commit into from
Jul 27, 2022

Conversation

vasicvuk
Copy link
Contributor

Implementing feature to read directories from provided directory path based on Idea here:
#253

Within the README.md I have also documented the readDir function which was not documented but available for usage.

@yxxhero
Copy link
Member

yxxhero commented Jul 21, 2022

@vasicvuk Please add unittest and e2etest for this.

@yxxhero
Copy link
Member

yxxhero commented Jul 21, 2022

@vasicvuk Thanks for your idea. I think readDirs is ambiguous. Do you have a higher function name

@yxxhero
Copy link
Member

yxxhero commented Jul 21, 2022

@vasicvuk e2e test in test/e2e/template/helmfile/tmpl_test.go

@vasicvuk
Copy link
Contributor Author

Hi @yxxhero,

Maybe listDir or readDirsWithinDir?

Do you have some name you would like?

@yxxhero
Copy link
Member

yxxhero commented Jul 22, 2022

@vasicvuk listDir will better. Can exec meet your needs?

@vasicvuk
Copy link
Contributor Author

vasicvuk commented Jul 22, 2022

@yxxhero As I understood exec is plarform specific and we are using helmfile on both Windows and Linux so it would be hard do handle both cases

@yxxhero
Copy link
Member

yxxhero commented Jul 22, 2022

@mumoshu WDYT?

@vasicvuk
Copy link
Contributor Author

@yxxhero Maybe listDir can list both files and folders instead just folders, i think that would be better

@yxxhero
Copy link
Member

yxxhero commented Jul 22, 2022

Can you describe your application scenario in detail

@vasicvuk
Copy link
Contributor Author

vasicvuk commented Jul 22, 2022

@yxxhero Sure,

We have multiple Cloud native apps built with 30-50 microservices, each application uses some of the common services we call platform and some services specific to their product.

All of the microservices have their own charts and since we standardize the helm chart with internal generator tool we have a file named global which values from are applied for every microservice.

Other then that we have a folder for each team (owner of multiple microservice), and also an override which we use to optimize the helmfile for certain client were we implement product to.

.
├── ...
├── base # Default configuration for the product
│   ├── global #  Global configuration
│   │   ├── values.yaml # Global values
│   │   └── 
│   ├── platform # Platform microservices
│   │   ├── service1.yaml # Platform microservice 1
│   │   ├── service2.yaml # Platform microservice 2
│   │   └── 
│   ├── app  # App microservices
│   │   ├── app1.yaml # App microservice 1
│   │   ├── app2.yaml # App microservice 2
│   │   └── 
│   └── 
├── override # Override configuration for the specific client 
│   ├── global #  Global override configuration
│   │   ├── values.yaml # Global values
│   │   └── 
│   ├── platform # Platform override microservices
│   │   ├── service1.yaml # Platform microservice 1
│   │   └── 
│   ├── app  # App override microservices
│   │   ├── app1.yaml # App microservice 1
│   │   └── 
│   └── 
└── ...

In override, the user can override only certain services, global values, or everything (it's up to the implementation team). Other than that we allow the option for an override file to begin with _ and then the microservice will not be deployed at all even if it is defined in base.

We implemented everything from the description above, but currently, we need to have a list of folders in YAML so we know what folders are there (app, platform, third-party) and we want to automate also this by having an option to list folders from code too.

@yxxhero
Copy link
Member

yxxhero commented Jul 22, 2022

@vasicvuk ok. Thanks very much. Perfect this PR according to your idea. I feel it is really useful.

@vasicvuk
Copy link
Contributor Author

@yxxhero, Thanks, I have changed the function to listDir and added unit tests for both readDir and listDir.

Since I am not really a GO developer, I need a little more time to understand how e2e part works, I am not sure if someone can jump in and help with e2e part?

Thanks

@vasicvuk
Copy link
Contributor Author

@yxxhero I managed to add e2e tests also :)

pkg/tmpl/context_funcs.go Outdated Show resolved Hide resolved
pkg/tmpl/context_funcs.go Outdated Show resolved Hide resolved
pkg/tmpl/context_funcs.go Outdated Show resolved Hide resolved
@yxxhero
Copy link
Member

yxxhero commented Jul 24, 2022

@mumoshu ping

docs/index.md Show resolved Hide resolved
@yxxhero
Copy link
Member

yxxhero commented Jul 26, 2022

"github.com/stretchr/testify/require"

@vasicvuk
Copy link
Contributor Author

@yxxhero Should I change whole file then, as everything is using deepEqual currently?

@yxxhero
Copy link
Member

yxxhero commented Jul 26, 2022

@vasicvuk whole file.

@vasicvuk
Copy link
Contributor Author

@yxxhero I changed all DeepEqual calls to use require now.

@yxxhero
Copy link
Member

yxxhero commented Jul 26, 2022

@vasicvuk Please sqush your commits. Thanks very much.

Signed-off-by: vasicvuk <vuk.vasic@asseco-see.rs>
@vasicvuk vasicvuk force-pushed the main branch 2 times, most recently from fd3b925 to 68d7c5f Compare July 26, 2022 14:21
@vasicvuk
Copy link
Contributor Author

@yxxhero Done

@vasicvuk vasicvuk changed the title Implement readDirs method Implement readDirEntries method Jul 26, 2022
Copy link
Member

@yxxhero yxxhero left a comment

Choose a reason for hiding this comment

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

LGTM

@yxxhero
Copy link
Member

yxxhero commented Jul 27, 2022

@vasicvuk please fix go test error

@vasicvuk
Copy link
Contributor Author

vasicvuk commented Jul 27, 2022

@yxxhero Can you help with identifying what the problem is, since i didn't change snapshot_test.go and it is showing this test as failure:

snapshot_test.go:126: helm push /home/runner/work/helmfile/helmfile/test/e2e/template/helmfile/raw-0.1.0.tgz oci://localhost:5000/myrepo: Error: failed to do request: Head "http://localhost:5000/v2/myrepo/raw/blobs/sha256:82d11697f6b3c7c5bcf619fb237648b2a8daee142dd0972f7f6244e72ea921e7": EOF
    snapshot_test.go:126: Unable to run helm push /home/runner/work/helmfile/helmfile/test/e2e/template/helmfile/raw-0.1.0.tgz oci://localhost:5000/myrepo: exit status 1
--- FAIL: TestHelmfileTemplateWithBuildCommand (10.77s)
    --- PASS: TestHelmfileTemplateWithBuildCommand/chart_need (6.33s)
    --- PASS: TestHelmfileTemplateWithBuildCommand/issue_2098_release_template_needs (0.07s)
    --- FAIL: TestHelmfileTemplateWithBuildCommand/oci_need (4.37s)

Seems to me that the local registry is not running and that it was a temporary test failure? Can we maybe retry the pipeline?

@mumoshu
Copy link
Contributor

mumoshu commented Jul 27, 2022

Thanks for your efforts!
Probably that's a flaky test added via #239. Let me rerun it and see if the error disappears.

@vasicvuk
Copy link
Contributor Author

@mumoshu Yes, now they are passing :)

@yxxhero yxxhero merged commit 9af8f12 into helmfile:main Jul 27, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants