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

Enhance Makefile and add editorconfig #115

Merged
merged 24 commits into from
Sep 23, 2019
Merged

Conversation

khos2ow
Copy link
Member

@khos2ow khos2ow commented Jun 24, 2019

Prerequisites

Put an x into the box(es) that apply:

  • This pull request fixes a bug.
  • This pull request adds a feature.
  • This pull request enhances existing functionality.
  • This pull request introduces breaking change.

For more information, see the Contributing Guide.

Description

This PR enhances the Makefile and its targets. List of changes:

  • add help target and colorize the output logs
  • add verify: Verify vendor dependencies
  • add fmt and checkfmt for go files formatting
  • add build-all which builds for all the specified OS/ARCH as well as sha256sum file
  • add release helper targets: major, minor and patch which auto upgrade version to corresponding logical next version
  • add tools which installs any required tools for development or CI jobs
  • add -mod=vendor and make sure the usage of Makefile targets will be universal, no matter if you're running it within GOPATH or outside of it.

Also added .editorconfig file to enforce unified file coding, line ending, tab/space sizes etc on all different IDEs and text-editors.

Added a checkfmt job on the CI pipeline as well to make sure we test code formatting on each PR as well. This means that if that job fails the formatting of go files are incorrect and can be fixed manually or by running make fmt from the root of the repo.

Issues Resolved

Fixes #67
Fixes #88

Checklist

Put an x into all boxes that apply:

Tests

  • I have added tests to cover my changes.
  • All tests pass when I run make test.

Documentation

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

Code Style

  • My code follows the code style of this project.

@metmajer
Copy link
Member

@khos2ow thank you for your submission! As I see it, your pull request addresses many diverse things that should be split up into dedicated submissions. Obvious candidates are the .editorconfig (which I find really useful), a new approach to versioning and creating releases, and potentially some more. I'd also like to you to take some time to explain this approach and why we need it.

@khos2ow
Copy link
Member Author

khos2ow commented Jun 30, 2019

To be honest I didn't want to create two PRs with only one file change in each (one for editorconfig and one for .golangci.yml), but I'll be happy to extract these to their own PRs if you see fit.

In terms of Makefile refactoring, those are the practices I've gathered over the while and potentially will be really useful. One of the main advantages is that you are not bound to be in GOPATH for the make targets to work (this is my own setup)

The release helper targets will potentially make the release process easier for the maintainers. When the time comes what needs to be done is essentially to decide which version bump is needed. Let's assume we want to go from v0.6.0 to v0.7.0 the only command needed is:

cd /path/to/local/clone
git checkout master
git pull # make sure the master is up to date
make minor push=true

This will bump the version, create a tag with that version and push it to remote.

And then release artifacts can be generated locally with make build-all compress=true and manually uploaded to github release page.

@khos2ow
Copy link
Member Author

khos2ow commented Aug 8, 2019

@metmajer do you want to proceed with this PR? or any suggestion on breaking it down into smaller PRs?

@metmajer
Copy link
Member

metmajer commented Aug 8, 2019

Yes, I do. Could you please...

  1. Adapt the .editorconfig to reflect the current indentation scheme? Right now, we use tabs, not spaces. Also, why use a tab size of 8 in the Makefile if you otherwise suggest 4 spaces? I suggest we use 4 everywhere.
  2. I am currently not convinced by the semi-automated versioning scheme. I just don't see the benefit over editing the VERSION file manually. Can we remove the versioning magic from this PR for now?

Thanks!

@khos2ow
Copy link
Member Author

khos2ow commented Aug 9, 2019

  1. That's true, actually another section should be added in .editorconfig for .go files to enforce tab, although gotfmt already takes care of this, but that should've been added.

  2. The reason I specifically set tab size of 8 for Makefile was to get the correct visual alignment. Because most of the editors consider tab size of 4 instead of actual 8. Consider the following for tab size of 4 and 8 for Makefile:

Screenshot at 2019-08-08 23-32-46

Screenshot at 2019-08-08 23-35-55

  1. IMO maintaining current version of binary in VERSION file is both redundant and error prone. Considering that we're releasing through git tags, and those are predictable and discoverable (programmatically) we don't really need to capture the same thing in a file.
    And also the process of releasing a new version will be slightly harder if we do have such a file. Maintainer needs to bump the version in that file, commit and push it, and then tag the branch.
    To be fair, if we choose to keep the VERSION file, it will still be easier to execute a make target (such as make minor) which does all the above rather than do them manually.

@khos2ow
Copy link
Member Author

khos2ow commented Aug 10, 2019

  • added .go in .editorconfig and set a tab size of 4 for it.
  • moved long script from Makefile to their own external files and removed the tab size of 8 for Makefile.

Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved

build-linux-amd64:
GOOS=linux GOARCH=amd64 $(GOBUILD) -o bin/linux-amd64/$(NAME)
.PHONY: lint
Copy link
Member

Choose a reason for hiding this comment

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

Please sort the make targets in each block.

Copy link
Member Author

Choose a reason for hiding this comment

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

+1

build-linux-amd64:
GOOS=linux GOARCH=amd64 $(GOBUILD) -o bin/linux-amd64/$(NAME)
.PHONY: lint
lint: ## Run linter
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure these comments are too helpful... lots of redundancy, hmm?

Makefile Outdated
#####################
## Release targets ##
#####################
.PHONY: release patch minor major
Copy link
Member

Choose a reason for hiding this comment

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

Let‘s keep the pattern of having a .phony above each target.

Copy link
Member Author

Choose a reason for hiding this comment

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

+1

Makefile Show resolved Hide resolved
@metmajer
Copy link
Member

Thanks for applying the changes, @khos2ow. Meanwhile, I have added some additional feedback. Aside, please...

  1. Rename the directory „hack“ to „scripts“.
  2. Should we invoke git-chglog as part of a release? Until now, I have used the Git commit message „Update Changelog.“, but it shows up in the Changelog itself. We should check, if specific (this) commit messages can be excluded in the git-chglog config. Can you look into this?

@khos2ow
Copy link
Member Author

khos2ow commented Aug 12, 2019

Should we invoke git-chglog as part of a release?

I really like the idea, it can be done in scripts/release/release.sh right after git tag --annotate ...., so immediately after releasing a tag we can generate and update CHANGELOG and push it too.

Update: actually if you're ok with this I'd like to fix this in another PR of its own, because it needs to take care of brew formula as well.

Makefile Outdated Show resolved Hide resolved
@metmajer
Copy link
Member

I totally agree with you on handling the brew formula update in a different PR, but a new release target should include an update of the Changelog. The updated Changelog should be contained in the tag, too.

@khos2ow
Copy link
Member Author

khos2ow commented Aug 13, 2019

I just revisited the doc of git-chglog and the easiest (and cleanest) way we can do, from now on, is to prefix commit with keywords, such as feat:, fix:, doc:, ... and only generate the changelog for subset of them. The changelog config of the git-chglog itself looks like this:

options:
  commits:
    filters:
      Type:
        - feat
        - fix
        - perf
        - refactor
  commit_groups:
    title_maps:
      feat: Features
      fix: Bug Fixes
      perf: Performance Improvements
      refactor: Code Refactoring
  header:
    pattern: "^(\\w*)\\:\\s(.*)$"
    pattern_maps:
      - Type
      - Subject

So instead of doing crazy hack to exclude Update Changlog commit, we can follow the convention mentioned above. What do you think about this approach Martin?

@metmajer
Copy link
Member

The problem with the approach you‘ve mentioned is that this project has never followed structured commits. This is why our current git-chglog looks the way it is. If we were to introduce these groups now, old commit messages would not be shown anymore. Is there no easier approach? What if we check for and ignore that commit message in the template? Also, I am not a fan of structuring commit messages. Thoughts?

@metmajer
Copy link
Member

Also, this hack can‘t be crazier than your version number hack in the Makefile 😀

@khos2ow
Copy link
Member Author

khos2ow commented Aug 13, 2019

Oh I see. I'm trying to see if I can make the config work to be able to generate the old commit message and generate new format from now on. If not successful manually remove the "Update Changelog" from the list.

@khos2ow
Copy link
Member Author

khos2ow commented Aug 13, 2019

Ok I've managed to make changelog both backward and forward compatible:

  • it keeps the previous changlog records intact
  • it considers <type>: <subject> as the new commit format (the same as git-chglog's Changelog itself)

@metmajer how do you want to proceed with this?

@metmajer
Copy link
Member

Awesome! That‘s the approach we need. Will you make your changes part of this PR? If we require specific commit message prefixes, we may have to amend CONTRIBUTING.md. Hm?

@khos2ow
Copy link
Member Author

khos2ow commented Aug 14, 2019

I think PR of its own would be better, because it will change .chglog/* and as you mentioned CONTRIBUTING.md.

Update: but to be frank, you don't need to follow the new commit title schema in your PRs, but the commits that go into master should follow them. So effectively 'squashed commit title' of a PR should follow that.

@metmajer
Copy link
Member

Agree that we don‘t need to change CONTRIBUTING.md. If you already have a Changelog template available, I‘d really prefer to have it in here to complete the release target. If more work will be needed, let‘s conclude the PR here and add the template in an upcoming PR very soon.

@khos2ow
Copy link
Member Author

khos2ow commented Aug 22, 2019

This commit 280835e will solve the template, but I prefer to have it in its own PR. If you feel like this is the correct place for it just proceed with this PR, otherwise let me know to revert that and open its own PR.

@khos2ow
Copy link
Member Author

khos2ow commented Aug 22, 2019

There's only one gotcha point, that we have to start using the new format for commit message <type>: <subject> from start of the next release, otherwise the changelog for current release (v0.7.0) might get corrupted.

@metmajer metmajer merged commit f0a7d37 into terraform-docs:master Sep 23, 2019
@khos2ow khos2ow deleted the makefile branch September 23, 2019 18:33
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.

terraform-docs --version output (dev build) #86 Integrate 'gofmt' and 'goimports' into Makefile
2 participants