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

Grunt s3 #1316

Merged
merged 19 commits into from
Jul 2, 2013
Merged

Grunt s3 #1316

merged 19 commits into from
Jul 2, 2013

Conversation

kenearley
Copy link

@pfiller @stof

Here's another take on #1315. Instead of committing the zip file to the repo, it's pushed to s3.

  • For pushing master to s3 - grunt merge - build all the files, create zip, push to s3, delete zip file.
    -- Workflow: merge to master, run grunt merge, commit and push to GitHub.
  • For pushing master and new version to s3 - grunt release - build all files, update download links in index.html create zip, push 2 zip files to s3, delete zip.
    -- Workflow: merge to master, update the version in package.json, run grunt release, commit and push to GitHub, create tag and push to GitHub.

@pfiller Of course, I will have to give you our s3 credentials when you're ready to test this out.

@@ -2,3 +2,7 @@
node_modules
.project
dist/
public/*.js
public/*.min.css
Copy link
Collaborator

Choose a reason for hiding this comment

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

as you are now ignoring all generated files, do we need to have both public and dist for it ?

Copy link
Author

Choose a reason for hiding this comment

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

The dist/ directory is the default setting for the build-gh-pages grunt plugin. I felt that it was safer to copy all needed files into that and run the gh-pages task. There is less chance of accidentally destroying non-generated files. It is deleted immediately after deploy to gh-pages.

@@ -2,3 +2,7 @@
node_modules
.project
dist/
public/*.js
public/*.min.css
grunt-aws.json
Copy link
Contributor

Choose a reason for hiding this comment

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

For some reason, grunt build fails if this file is not present. We should include a version (without credentials) in master.

@pfiller
Copy link
Contributor

pfiller commented Jul 1, 2013

Thanks @kenearley. This is pretty nice stuff.

I think I prefer this approach to #1315. I'm a little nervous about pulling the JS out of the repo, but I think this will help reduce PR noise (both in our diffs and in people opening PRs without coffee changes). I made a few notes, but I don't think this is too far from merge ready.

@koenpunt I'd welcome your feedback on this PR as well. It solves an issue you brought up in #1314.

@koenpunt
Copy link
Collaborator

koenpunt commented Jul 2, 2013

I think this is a great solution, as people who only want to use the library don't need the source files, and the ones that want to modify something in Chosen probably know how to clone and use/build the sources.

@kenearley
Copy link
Author

@pfiller

  • I've updated the names of the S3 tasks
  • pushing the new version to S3 now happens when pushing out the gh-pages.
  • build task no longer fails if you don't have s3 credentials

Note: The biggest weakness with this workflow is that you have to remember to run the grunt task master-s3 after every push to the master branch.

Conflicts:
	public/chosen.jquery.js
	public/chosen.jquery.min.js
	public/chosen.min.css
	public/chosen.proto.js
	public/chosen.proto.min.js
@koenpunt
Copy link
Collaborator

koenpunt commented Jul 2, 2013

Maybe a local githook can solve that problem?

@pfiller
Copy link
Contributor

pfiller commented Jul 2, 2013

Maybe a local githook can solve that problem?

Certainly a possibility. I think we can live with the annoyance for the time being, though.

@kenearley
Copy link
Author

Maybe a local githook can solve that problem?

I briefly looked into that. I'd like to dig further, eventually ;)

@koenpunt
Copy link
Collaborator

koenpunt commented Jul 2, 2013

Today Github announced a 'Releases' feature: https://github.com/blog/1547-release-your-software maybe that can be another possibility for releasing the zips?

@kenearley
Copy link
Author

Ha! Great timing!

@pfiller
Copy link
Contributor

pfiller commented Jul 2, 2013

That GitHub releases thing looks interesting, but there doesn't really seem to be a way to automate it. I think we'll merge this as-is and do some experimentation with releases this week. It's possible we can use a little of column A and a little of column B.

In the meantime, merging this will make reviews a lot easier. So it's a big +1 from me.

Conflicts:
	public/chosen.jquery.js
	public/chosen.jquery.min.js
	public/chosen.proto.js
	public/chosen.proto.min.js
pfiller added a commit that referenced this pull request Jul 2, 2013
Remove generated assets from repo and add a grunt task to push to S3
@pfiller pfiller merged commit 771867c into master Jul 2, 2013
@pfiller pfiller deleted the grunt-s3 branch July 2, 2013 21:07
pfiller added a commit that referenced this pull request Jul 2, 2013
Remove generated assets from repo and add a grunt task to
push to S3 #1316
@pfiller
Copy link
Contributor

pfiller commented Jul 2, 2013

Incidentally, the release function makes for a really nice looking changelog. I've added the past few tags and will probably add a few more.

https://github.com/harvesthq/chosen/releases

@kenearley
Copy link
Author

Nice!

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.

4 participants