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

Clean build previous artifacts after successful build (II) #108

Merged
merged 13 commits into from
Jan 31, 2021

Conversation

philip-peterson
Copy link
Contributor

This is a rebase, refactoring, and extension of PR #69. Thanks to @hamza1311 for starting the initial draft. Unfortunately during the rebase the changes @hamza1311 made were lost because the file they were in has been deleted upstream, but let's be sure that attribution makes it through this PR.

Happy to answer any questions. The main takeaway is that we now have two kinds of dist directory: staging and final. Plugins should output to staging, and Trunk itself moves things from staging to final. In theory, a plugin could put files in final itself which is pretty undesirable. If we move to untrusted plugins, this might be a good action to restrict.

Checklist

  • Updated CHANGELOG.md describing pertinent changes.
  • Updated README.md with pertinent info (may not always apply).

@philip-peterson
Copy link
Contributor Author

Have not tested on Windows yet.

Copy link
Member

@thedodd thedodd left a comment

Choose a reason for hiding this comment

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

Awesome work, @philip-peterson! Thanks for all of the effort here, and definitely shoutout to @hamza1311 for their work as well!

Just a few small things I'm requesting changes for. In addition to the line items I've specified, what do you think about updating the various context/logging statements to say "staging dir" instead of "dist/.current"? I'm just thinking that keeping it generic will help to avoid confusion if we decide to change the name of the staging dir in the future.

src/build.rs Outdated Show resolved Hide resolved
src/build.rs Outdated Show resolved Hide resolved
src/build.rs Outdated Show resolved Hide resolved
src/config/rt.rs Show resolved Hide resolved
Copy link
Member

@thedodd thedodd left a comment

Choose a reason for hiding this comment

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

One small change in functionality, and this thing should be ready to rock! Looks like a rebase is needed now as well.

@philip-peterson I'll make sure not to merge anything else until this lands, just to avoid excessive rebasing.

src/build.rs Outdated Show resolved Hide resolved
Copy link
Member

@thedodd thedodd left a comment

Choose a reason for hiding this comment

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

+1

@philip-peterson && @hamza1311 thanks for all of the work on this PR. I'll update the changelog tonight and get things prepped for a new release.

@thedodd thedodd merged commit a4e5661 into trunk-rs:master Jan 31, 2021
@thedodd thedodd mentioned this pull request Feb 1, 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.

3 participants