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

Pages target dir #1170

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

adrianbroher
Copy link

@adrianbroher adrianbroher commented Feb 23, 2020

This PR tries to take a stab at #1164.

Never did anything related to ruby, so feedback is appreciated.

Copy link
Contributor

@svenfuchs svenfuchs left a comment

Choose a reason for hiding this comment

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

this looks quite good to me @adrianbroher! i am wondering if the log output will be very useful if we omit the work dir from it, and i'm not sure we need the dst_dir method at all if it's just eqal to target_dir, but those are rather cosmetic questions.

have you been able to test this with an actual deployment?

you can trigger a deployment with your fork like so:

deploy:
- provider: pages
  edge: 
    repo:  adrianbroher/dpl
    branch: pages-target-dir
    # ...

git_clone: 'Cloning the branch %{target_branch} from the remote repo',
git_init: 'Initializing local git repo',
git_checkout: 'Checking out orphan branch %{target_branch}',
copy_files: 'Copying %{src_dir} contents to %{work_dir}',
copy_files: 'Copying %{src_dir} contents to %{dst_dir}',
Copy link
Contributor

Choose a reason for hiding this comment

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

i think this can just be target_dir, in which case we wouldn't need the extra method dst_dir below.

@adrianbroher
Copy link
Author

adrianbroher commented Feb 26, 2020

am wondering if the log output will be very useful if we omit the work dir from it

I can add another message to the setup method.

and i'm not sure we need the dst_dir method at all if it's just eqal to target_dir

I actually wanted to add a guard/limitation so that target_dir can only be a descendant directory of work_dir. However I have yet to find out how to implement this in ruby and if the accessor is the right place for this or if this opt thing allows validators.

have you been able to test this with an actual deployment?

No, but I created this test repository.

@adrianbroher
Copy link
Author

While looking at the log of the build it may be a good idea to call git add -A %{dst_dir} instead of git add -A ..

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.

2 participants