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

deps: update to golang v1.21 #96

Merged
merged 20 commits into from
Feb 1, 2024

Conversation

Monirzadeh
Copy link
Collaborator

@Monirzadeh Monirzadeh commented Dec 28, 2023

try to update golang v1.14 to 1.21

  • replace io/ioutil
  • find all deprecated library
  • update workflows

@Monirzadeh Monirzadeh marked this pull request as ready for review December 28, 2023 16:27
@Monirzadeh
Copy link
Collaborator Author

Monirzadeh commented Dec 28, 2023

Hi @fmartingr
i try to not change too much but please review this PR.

  • please check .golangci.yml do we need specific config?
  • reflect.SliceHeader and reflect.StringHeader deprecated i change function and the result is the same on my tests. do you know why we change string to byte that specific way?

@fmartingr
Copy link
Member

Hi @fmartingr i try to not change too much but please review this PR.

Not that I am aware of, I'd say let's use the same in all projects so results are the same.

  • reflect.SliceHeader and reflect.StringHeader deprecated i change function and the result is the same on my tests. do you know why we change string to byte that specific way?

I'm assuming they used it that way to avoid memory allocation as the comment suggest, never did something like that. We may need to add some benchmarks, but I wouldn't worry too much about that for now.

Now that we are on it, I think we can bump golang to 1.21 since 1.22 will release in February and 1.20 will become obsolete.

@Monirzadeh Monirzadeh changed the title update to golang v1.20 update to golang v1.21 Dec 28, 2023
@fmartingr fmartingr self-requested a review December 28, 2023 18:16
@Monirzadeh
Copy link
Collaborator Author

Monirzadeh commented Dec 28, 2023

i review most of the workflows and remove what i think we don't need.
currently keep golang-lint and add unit-test later when i add some unit-test for the project.
change workflow golang-lint for new pull request.
please run it manually to be sure everything work fine (if it possible).

side-note: we have some PR from dependencies not bad if you can review them.
if you want i can fix them in this PR too

@fmartingr
Copy link
Member

Why did you remove all linters and its configuration files from the workflows? You only left golangci-lint but removed both configuration files for it.

Maybe I expressed myself wrong, with my previous comment I meant to use the same yaml file for golangci-lint, but I think we should leave the other linters as well unless you have found they are not useful to this project. But they seem useful to me at a first glance (all but the shellcheck, since there does not seem to be shell files in the repo).

What do you think?

@fmartingr fmartingr changed the title update to golang v1.21 deps: update to golang v1.21 Dec 30, 2023
@fmartingr
Copy link
Member

Also, update the dependabot.yml file with the same we have in shiori: https://github.com/go-shiori/shiori/blob/master/.github/dependabot.yml

This will make it group all updates in the same PR, which is easier to merge than a bunch of separate ones :)

@Monirzadeh
Copy link
Collaborator Author

Monirzadeh commented Dec 30, 2023

I prefer the way we do golangci instead of being limited to the specific test, so I remove config of golangci.
I remove spellcheck, docker and bash as you mention we don't have them in the project.
Maybe I misunderstood this line.

I'd say let's use the same in all projects

So I try to keep things we have in shiori.
I will return markdown and misspelling, but do you think we need action-alex?

@fmartingr
Copy link
Member

Yeah, sorry, I was referring to golangci-lint specifically.

Please revert the changes for all linters but the shellcheck one, since I'm not aware of any bash/sh code in this repository.

@Monirzadeh Monirzadeh self-assigned this Dec 31, 2023
@Monirzadeh
Copy link
Collaborator Author

Monirzadeh commented Jan 4, 2024

@fmartingr
I return alex misspell and super-linter for markdown.
Remove spell-check and from super-linter two things docker and bash (we don't have docker file too).
Update code-ql too.

@Monirzadeh
Copy link
Collaborator Author

Monirzadeh commented Jan 4, 2024

I think it is ready now 👍

Copy link
Member

@fmartingr fmartingr left a comment

Choose a reason for hiding this comment

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

Hey, left you some things around.

Also i would bring back the golangci.yml file since it has specific configuration that may be there for some reason, until we figure out what we need and what dont. Is there any speciifc linter or configuration causing false positives or something like that that I should be aware of?

.github/workflows/linter.yml Show resolved Hide resolved
utils.go Outdated Show resolved Hide resolved
@Monirzadeh
Copy link
Collaborator Author

Monirzadeh commented Jan 13, 2024

Hey, left you some things around.

Also i would bring back the golangci.yml file since it has specific configuration that may be there for some reason, until we figure out what we need and what dont. Is there any speciifc linter or configuration causing false positives or something like that that I should be aware of?

in general i try to we have same settings in all shiori projects + specific needs for specific project
as i know review that before default config for golangci-lint is here

Things we don't need in the old config and fix in the default config.

  1. In the old config, structcheck' and varcheck' are abandoned and we use unused instead in the default config.
  2. in the old config rowserrcheck added to the old. that i find that not useful that is for sql (as i see we don't use sql in obelisk) you can check that here. in default config we don't have rowserrcheck

Exclude things in old config that we have in the default config.

  1. gosec with config that exclude G108, I think it is better to find solution that exclude that if we have problem on that.
      - G108 # Profiling endpoint automatically exposed on /debug/pprof

what do you think? you can read more about that here

  1. goconst that i think excluding this can't be a good idea you can read about that here but in general
Finds repeated strings that could be replaced by a constant.
  1. dupl is the thing that maybe give us false positive we can return that if you want.

Specific settings in the old config that I think most of them are not necessary

  1. errcheck that force this
   # report about assignment of errors to blank identifier: `num, _ := strconv.Atoi(numStr)`;
    # default is false: such cases aren't reported by default.
    check-blank: true

I think it is not a big deal that we do other Shiori project.(if you want I can check the project for error handling in other PR too). should i return that?

  1. govet with active check for shadowed variables (maybe useful do you want to active this option?)

  2. we have gocyclo that increase min-complexity from 10 to 15 you can read about that here. i think this config just there because maybe some function is not good enough so i think lower variable (default 10) is better.

  3. misspell that force to ignore-words: - someword do you think we need that? we have reviewdog/action-misspell too in workflow.

  4. maybe we want keep spellcheck config. do we need that? you can read about that here

  stylecheck:
    go: "1.16"
    # https://staticcheck.io/docs/options#checks
    checks: [ "all", "-ST1003", "-ST1008", "-ST1016" ]
    # https://staticcheck.io/docs/options#initialisms
    initialisms: [ "ACL", "API", "ASCII", "CPU", "CSS", "DNS", "EOF", "GUID", "HTML", "HTTP", "HTTPS", "ID", "IP", "JSON", "QPS", "RAM", "RPC", "SLA", "SMTP", "SQL", "SSH", "TCP", "TLS", "TTL", "UDP", "UI", "GID", "UID", "UUID", "URI", "URL", "UTF8", "VM", "XML", "XMPP", "XSRF", "XSS" ]

What do you think? based on this which part you want to keep?
Sorry for the long comment, I try to cover all settings if I am not missing something.

@fmartingr
Copy link
Member

Sorry for the long comment, I try to cover all settings if I am not missing something.

Don't be sorry, thanks a lot for your thorough investigation into this. Let's go with your changes since all points you mention seems fair to me. We can always change it later if we think it's necessary anyway. Awesome work!

Co-authored-by: Felipe Martin <812088+fmartingr@users.noreply.github.com>
@fmartingr fmartingr merged commit e669ef7 into go-shiori:master Feb 1, 2024
5 checks passed
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.

None yet

2 participants