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

Feature Ignore Paths #64

Merged
merged 6 commits into from
Jun 23, 2021

Conversation

joelholmes
Copy link
Contributor

Issue: #37

Created a function that will copy existing files listed in the destination directory to the staging area prior to deletion preserving the content. I felt that pulling the content into the staging area was simple compared to making the Replace function more complicated. Also I felt that the issue described only keeping the specified files where the alternative solution would keep untracked files around.

Testing seemed a little tricky and am open to suggestions for a separate test. Essentially I changed a value in a file and rely on the examples tests.

@vmwclabot
Copy link

@joelholmes, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction.

@jhosteny
Copy link

jhosteny commented May 13, 2021

@joelholmes just making a note that I believe this works with your PR if you make the following change to the example:

apiVersion: vendir.k14s.io/v1alpha1
kind: Config
directories:
- path: components/terraform
  contents:
  - path: account-map
    git:
      url: https://github.com/cloudposse/terraform-aws-components
      ref: 0.137.0
    newRootPath: modules/account-map
    includePaths:
      - modules/account-map/**/*
    ignorePaths: &ignore
    - account-map/backend.tf.json

Note that the ignorePaths glob has the account-map in the path. Instead, I think this should assume that it is rooted in the destination directory (i.e., ./components/terraform/account-map), so that the provided example would work. Perhaps @aknysh can confirm?

@joelholmes
Copy link
Contributor Author

I was able to get this to work with the relative path and have updated the code.

@cppforlife
Copy link
Contributor

will try to take a look at this PR this coming week.

Copy link
Contributor

@cppforlife cppforlife left a comment

Choose a reason for hiding this comment

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

first pass review.

pkg/vendir/directory/directory.go Outdated Show resolved Hide resolved
pkg/vendir/directory/directory.go Outdated Show resolved Hide resolved
pkg/vendir/directory/directory.go Outdated Show resolved Hide resolved
pkg/vendir/directory/staging_dir.go Outdated Show resolved Hide resolved
pkg/vendir/directory/staging_dir.go Outdated Show resolved Hide resolved
pkg/vendir/directory/staging_dir.go Outdated Show resolved Hide resolved
pkg/vendir/directory/staging_dir.go Outdated Show resolved Hide resolved
pkg/vendir/directory/staging_dir.go Outdated Show resolved Hide resolved
pkg/vendir/directory/staging_dir.go Outdated Show resolved Hide resolved
}

// Create reference point from staging path to root
rootPath := strings.Replace(stagingPath, d.stagingDir, rootDir, 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

im still chewing on this line. will play around more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, keep me posted on what you would suggest. I messed around with this a bit and was the best solution I could come up with. Not super happy with the solution.

@vmwclabot
Copy link

@joelholmes, your company's legal contact did not review your signed contributor license agreement within the 14 day limit. The merge can not proceed. Click here to resign the agreement.

@joelholmes
Copy link
Contributor Author

@joelholmes, your company's legal contact did not review your signed contributor license agreement within the 14 day limit. The merge can not proceed. Click here to resign the agreement.

I was able to resign the agreement.

@joelholmes
Copy link
Contributor Author

Our legal department has not received the CLA email and emails to OSSContributions@vmware.com have bounced. Is there another way or a contact we can reach out to in order to resolve this?

@microwavables
Copy link
Member

Our legal department has not received the CLA email and emails to OSSContributions@vmware.com have bounced. Is there another way or a contact we can reach out to in order to resolve this?

Hey Joel, wanted to give you an update to let you know we are working to resolve the issues you are experiencing with the CLA and email. Will let you know as soon as things are resolved.

@aknysh
Copy link
Contributor

aknysh commented Jun 4, 2021

@joelholmes just making a note that I believe this works with your PR if you make the following change to the example:

apiVersion: vendir.k14s.io/v1alpha1
kind: Config
directories:
- path: components/terraform
  contents:
  - path: account-map
    git:
      url: https://github.com/cloudposse/terraform-aws-components
      ref: 0.137.0
    newRootPath: modules/account-map
    includePaths:
      - modules/account-map/**/*
    ignorePaths: &ignore
    - account-map/backend.tf.json

Note that the ignorePaths glob has the account-map in the path. Instead, I think this should assume that it is rooted in the destination directory (i.e., ./components/terraform/account-map), so that the provided example would work. Perhaps @aknysh can confirm?

I think both ways are OK, but
ignorePaths has a diff behavior than includePaths.
includePaths define the source paths, while ignorePaths works with the destination paths.
And since we already define the destination folder in - path: account-map, then I think there is no need to use the same folder account-map in ignorePaths

@microwavables
Copy link
Member

Our legal department has not received the CLA email and emails to OSSContributions@vmware.com have bounced. Is there another way or a contact we can reach out to in order to resolve this?

Hey Joel, wanted to give you an update to let you know we are working to resolve the issues you are experiencing with the CLA and email. Will let you know as soon as things are resolved.

@joelholmes my sources tell me to advise you to try oss.contributions.vmware@vmware.com as an alternate email while we investigate what's going on with osscontributions@vmware.com.

Fix name in error message
@vmwclabot
Copy link

@joelholmes, your company's legal contact has approved your signed contributor license agreement. It will also be reviewed by VMware, but the merge can proceed.

@vmwclabot
Copy link

@joelholmes, VMware has approved your signed contributor license agreement.

@osterman
Copy link

Anxiously awaiting approval and merge =)

@osterman
Copy link

@cppforlife is this good to go?

@ewrenn8 ewrenn8 merged commit 8a1b915 into carvel-dev:develop Jun 23, 2021
@osterman osterman mentioned this pull request Jun 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants