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

Add a option to include files when tag #55

Closed
filenwind opened this issue Jun 27, 2018 · 15 comments
Closed

Add a option to include files when tag #55

filenwind opened this issue Jun 27, 2018 · 15 comments

Comments

@filenwind
Copy link

filenwind commented Jun 27, 2018

Some times I have files that only want keep in Tag.

Like dist folder, I don't want track in any branch, so I add /dist to .gitignore

But I want dist keep in Tags to release.

By assets document described, add dist to assets is no help.

If a file has a match in .gitignore it will always be excluded.

So, can add a option to include files only when doing tag?

Or have some way to achieve?

Thank you!

@pvdlg
Copy link
Member

pvdlg commented Jun 27, 2018

What "keep in Tag" means? What git command would you run to achieve what you are asking?

@filenwind
Copy link
Author

Sorry I didn't describe it very clearly.

In manual, below steps can achieve what I want:

1. Checkout to a new branch XXX
2. build files to ./dist folder
3. force add ./dist folder and commit (because ./dist is in .gitignore) 
4. create a tag so this tag can include ./dist files
5. switch branch back to where you come from
6. delete branch XXX

So in simply, I want ignore some files, but can include in Tag.

At first I thought option assets can do what I want, until I saw the document.

Can this done by semantic-release/git automatically?

Or it doesn't make sense...

Thanks for you help!

@pvdlg
Copy link
Member

pvdlg commented Jun 27, 2018

We can't include files in a Tag, as a tag is only a reference to a commit. The files would be added to the commit (which in turn is referenced by the tag).

Currently we use the command git add --ignore-errors to add the file to be committed.

I guess we can use git add --ignore-errors --force instead, so all files referenced in the assets option would be added to the commit, no matter if they are in .gitignore or not.
I don't think we need an option for that, we can adopt this behavior by default and mention in the doc that everything in assets will be included no matter what's in .gitignore, as it's reasonable to assume that a user adding files in assets actually want them committed.

Would that work for you?

@filenwind
Copy link
Author

filenwind commented Jun 27, 2018

Not 100% but almost there.

I think make --force be optional will be great, can keep original behavior and create a new one.

Maybe... forceAssets: boolean (default false)?

Sorry I think again, you are right, user adding files in assets actually want them committed.

Thanks!

@pvdlg
Copy link
Member

pvdlg commented Jun 27, 2018

I'm not convinced about adding yet another option.

In which case one would set that option to false?
Doing so would only allow cases where you have dist in your .gitignore and in assets but don't want to include it. If you don't want to include some files then don't add them to assets.

@filenwind
Copy link
Author

Yes, sorry I just lost my brain 😆

@semantic-release-bot
Copy link
Collaborator

🎉 This issue has been resolved in version 6.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@MatthewMi11er
Copy link

MatthewMi11er commented Jul 5, 2018

Just saw this, and I think your assumption here is faulty. As an example, here is the relevant line of my config:

      {
        "path": "@semantic-release/git",
        "assets": ["**"]
      }

I want to include in the commit everything that's changed EXCEPT for what's in the .gitignore. For me the new behavior is unwelcome as I now either need to duplicate my .gitignore in the semantic-release config or I need to be overly explicit about what I want to add.

Not sure the best way to resolve this. NPM does it by having an .npmignore which overrides .gitignore if and only if .npmignore is present.

@pvdlg
Copy link
Member

pvdlg commented Jul 6, 2018

I understand your point, however I don't think we want to encourage the practice of configuring assets with something too loose, for several reasons:

  • Files that match are going to be published, so if by mistake your build generate a temporary file at the root of the project and contains sensitive data they would be exposed, without you noticing
  • The commit done by the plugin is named as a release commit and is meant to contain a release, not code changes, so with a loose assets you might end up committing code changes by mistake

You should configure your project and build steps to clearly separate what is a build output from the source code. The case of npm is different though because the deliverable can be and often are the same thing. In the case of this plugin, the source code is never considered being part of the release as it is already checked in the repo.
The point of this plugin is to push to the repo only the output of the build, because everything else is already in the repo.

@MatthewMi11er
Copy link

Your point is well taken, but part of my build step is to run code cleanup, eg. eslint. If the clean up fails then the build should fail, but if it is able to autofix any issues then there's no reason not to pass the build. If it passes the build and fixed code formatting issues, it should commit those changes along with the updates to the change log.

@MatthewMi11er
Copy link

Also, I think the fact that assets supports globs in the first place contradicts the desire for specificity. As you mentioned the desire is to not mistakenly include things you don't want in the commit. The normal way to do that is .gitignore, so now this change breaks that assumption and along with the globs makes it MORE likely that something incorrect gets committed.

@pvdlg
Copy link
Member

pvdlg commented Jul 6, 2018

Linting and fixing code shouldn't be part of a release process. That should be part of the development process. All the code that reach master is releasable and therefore should pass the linter validation.
In addition relying on eslint --fix without human review and running tests is dangerous as it might fix the violation in an unexpected way. eslint --fix is more a development tool and not as a build tool.

You should configure your CI/Linter to run on PRs and report errors there, so you can fix the linting errors before merging the faulty code into master. If for some reasons you don't want to install an eslint plugin in your editor (that would run eslint --fix each time you save), you can run that in the CI for each PR and commit the fixed files on the feature branch. But frankly it's a lot easier to use an editor plugin or a git hook on commit rather than commiting from the CI.
It would also make things a lot clearer to contributors of your project, as having your code modified on the CI without you being aware is really confusing.

Long story short, the workflow you are describing is something we would consider a bad practice that goes against the type of workflow semantic-release try to support. So we won't implement a change to make it easier to support such workflow.

Regarding your second comment, supporting globs doesn't goes against specificity. You can still configure a glob and be specific (glob does not only means **). For example if a project has 100s of files that have to be transpiled it would be pretty annoying to have to add 100s of entries in the assets config. It would be even more difficult to manage as you would have to change the config every time you add a file to your project.

In such case you would have a src directory with al the source files and a dist directory to which your write the transpiled files. You would add dist to .gitignore so the files generated during development wouldn't be checked in the repo. And you would configure assets to dist/** to automatically release the build files.
Maybe the build process also generate .map files that are useful in dev but you don't want to release them. In that case you can assets to dist/**/*.{js,css} for example.

@MatthewMi11er
Copy link

MatthewMi11er commented Jul 7, 2018

I wasn't suggesting that you would want to add 100s of entries individually, but rather if you are adding 100s of entries using globs, you will inevitably need a way to exclude files that would otherwise be included. With git the normal way of doing that is .gitignore but this change breaks the normal paradigm (unnecessarily in my opinion).

@pvdlg
Copy link
Member

pvdlg commented Jul 7, 2018

As mentioned in the documentation you can use negative globs to exclude some files:

[['dist', '!**/*.css']]: include all files in the dist directory and its sub-directories excluding the css files.

[['dist', '!**/*.css'], 'package.json']: include package.json and all files in the dist directory and its sub-directories excluding the css files.

[['dist/**/*.{js,css}', '!**/*.min.*']]: include all js and css files in the dist directory and its sub-directories excluding the minified version.

A Git repository is primarily meant to store source code and the .gitignore file is meant to ignore files that doesn't belong to the source code. This plugin essentially "abuse" the Git repository as a way to store releases. The files that you want to exclude to from your source code and the one you want to exclude from a release are two different notions that doesn't necessarily overlap.

This is why npm offers .npmignore for example. That allow to define both the source code to check in the repo (.gitignore) and the one to include in a release (files in package.json + .npmignore).
Same thing with this plugin which now allows to define both the source code to check in the repo (.gitignore) and the one to include in a release (assets with optionally negative globs).

@MatthewMi11er
Copy link

I'll take your word for it that this is an "abuse" of git, but I guess we're just back to my original comment. "Abuse" or not we are using git, and I've already told git what I want to ignore. While negative globs work, they require that I repeat configuration. Anyway that's my two cents on the matter, overall semantic-release is pretty awesome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants