-
Notifications
You must be signed in to change notification settings - Fork 365
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
base: master
Are you sure you want to change the base?
Pages target dir #1170
Conversation
There was a problem hiding this 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}', |
There was a problem hiding this comment.
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.
I can add another message to the
I actually wanted to add a guard/limitation so that
No, but I created this test repository.
|
While looking at the log of the build it may be a good idea to call |
This PR tries to take a stab at #1164.
Never did anything related to ruby, so feedback is appreciated.