search

nephio-experimental / one-summit-22-workshop

Apache License 2.0
5 stars 34 forks source link

nephio-in-docker (nind) #113

Closed danaschwanden closed 1 year ago

danaschwanden commented 1 year ago

nephio-in-docker (nind) packages the nephio-ansible-install into a Docker container following the setup used in the nephio ONE summit 2022 workshop. In addition nind also runs a VS Code OSS workstation instance providing a browser based development and test environment.

Change log:

Core changes for new NIND feature
Cosmetic changes to documentation
Changes to please the Linter
Changes to TF code (run and tested the instance creation after changes)
Indentation/leading/trailing spaces removed
Double quotes added
vjayaramrh commented 1 year ago

@danaschwanden Would be great if you pushed a PR just with changes related to NIND only, Thanks. It would make it easier to review.

danaschwanden commented 1 year ago

@danaschwanden Would be great if you pushed a PR just with changes related to NIND only, Thanks. It would make it easier to review.

Hi @vjayaramrh , I had to change the non NIND files to make the linter happy. If I only include the NIND related changes then the linter spits the dummy. Would you still like me to reverse those changes?

vishwanathj commented 1 year ago

@danaschwanden Would be great if you pushed a PR just with changes related to NIND only, Thanks. It would make it easier to review.

Hi @vjayaramrh , I had to change the non NIND files to make the linter happy. If I only include the NIND related changes then the linter spits the dummy. Would you still like me to reverse those changes?

@danaschwanden Thanks for clarifying. Alternate Suggestion (if it makes sense) -> Do you mind calling out in the PR body the list of files specific to NIND feature that needs review and add a note that the other files have been modified to pass the linter and have no actual content changes. Thanks

danaschwanden commented 1 year ago

@danaschwanden Thanks for clarifying. Alternate Suggestion (if it makes sense) -> Do you mind calling out in the PR body the list of files specific to NIND feature that needs review and add a note that the other files have been modified to pass the linter and have no actual content changes. Thanks

Thanks a lot @vjayaramrh , had a quick exchange with @s3wong on slack earlier today and shared the following high level assessment with him:

Long story short: The 4 files outside the NIND context boil down to the following:

Can add that to the first comment above to make it more visible. Please lemme know if you think this would be helpful?

henderiw commented 1 year ago

I tested the complete flow with Dan's files and did some further changes, but I believe they are extensions from what Dan did. In general I like the nind approach, as it make the installation portable by using the container environment.

I tested it on my laptop + my own VM in the cloud and in the google workstation.

vjayaramrh commented 1 year ago

@danaschwanden Thanks for clarifying. Alternate Suggestion (if it makes sense) -> Do you mind calling out in the PR body the list of files specific to NIND feature that needs review and add a note that the other files have been modified to pass the linter and have no actual content changes. Thanks

Thanks a lot @vjayaramrh , had a quick exchange with @s3wong on slack earlier today and shared the following high level assessment with him:

* 4 out of the 35 files are outside the NIND context (not tested by me so far) and updated to keep the linter happy

* 31 files are related to NIND (tested implicitly by running NIND), of which 8 are net new (incl Dockerfile and pictures, etc)

* Most of the linter driven changes are removing whitespaces in curly brackets which the linter seems to particularly dislike

Long story short: The 4 files outside the NIND context boil down to the following:

* 2 TF files (infra/compute_instances.tf and infra/output.tf) would benefit from some testing. Reckon running the TF code will quickly tell whether it is all good or something has been broken.

* The other two files (from the ansible_kind folder) appear to be fairly benign changes as they are mostly removing spaces.

Can add that to the first comment above to make it more visible. Please lemme know if you think this would be helpful?

@danaschwanden Thanks for clarifying. Alternate Suggestion (if it makes sense) -> Do you mind calling out in the PR body the list of files specific to NIND feature that needs review and add a note that the other files have been modified to pass the linter and have no actual content changes. Thanks

Thanks a lot @vjayaramrh , had a quick exchange with @s3wong on slack earlier today and shared the following high level assessment with him:

* 4 out of the 35 files are outside the NIND context (not tested by me so far) and updated to keep the linter happy

* 31 files are related to NIND (tested implicitly by running NIND), of which 8 are net new (incl Dockerfile and pictures, etc)

* Most of the linter driven changes are removing whitespaces in curly brackets which the linter seems to particularly dislike

Long story short: The 4 files outside the NIND context boil down to the following:

* 2 TF files (infra/compute_instances.tf and infra/output.tf) would benefit from some testing. Reckon running the TF code will quickly tell whether it is all good or something has been broken.

* The other two files (from the ansible_kind folder) appear to be fairly benign changes as they are mostly removing spaces.

Can add that to the first comment above to make it more visible. Please lemme know if you think this would be helpful?

@danaschwanden appreciate the response and the details you have provided in the PR body. It is very helpful.

vjayaramrh commented 1 year ago

@johnbelamaric Would it make sense to create a tag prior to this PR getting merged that has all the working changes till December?