Skip to content

refactor: rename connection.RateLimit to connection.RateLimitPerHour#2492

Merged
warren830 merged 7 commits into
apache:mainfrom
perhapzz:main
Jul 14, 2022
Merged

refactor: rename connection.RateLimit to connection.RateLimitPerHour#2492
warren830 merged 7 commits into
apache:mainfrom
perhapzz:main

Conversation

@perhapzz
Copy link
Copy Markdown
Contributor

Summary

Description

rename connection.RateLimit to connection.RateLimitPerHour

Does this close any open issues?

close #2360

Copy link
Copy Markdown
Contributor

@likyh likyh left a comment

Choose a reason for hiding this comment

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

Maybe you can add some new migration scripts.

Name string `gorm:"type:varchar(100);uniqueIndex" json:"name" validate:"required"`
Endpoint string `mapstructure:"endpoint" env:"GITHUB_ENDPOINT" validate:"required"`
Proxy string `mapstructure:"proxy" env:"GITHUB_PROXY"`
RateLimitPerHour int `comment:"api request rate limit per hour"`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No, you can not change the archived file. It will break existing magrations.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No, you can not change the archived file. It will break existing magrations.
@likyh
How about we don't add the migration script this time, just so v0.12 doesn't affect it, and we'll add it in a subsequent version?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

or we need to change version.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Change the version value of the migration script to a newer value is sufficient before next release

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Keep going, K-ON @phzbbbbbbbb 😹

@abeizn
Copy link
Copy Markdown
Contributor

abeizn commented Jul 14, 2022

@phzbbbbbbbb can you change the version to lastest time in plugin init_schema. such as 20220707231236 --> 20220714201138 . if not, it might fail. someone who had run the devlake service, due to the database model updated.

image

Name string `gorm:"type:varchar(100);uniqueIndex" json:"name" validate:"required"`
Endpoint string `mapstructure:"endpoint" env:"GITHUB_ENDPOINT" validate:"required"`
Proxy string `mapstructure:"proxy" env:"GITHUB_PROXY"`
RateLimitPerHour int `comment:"api request rate limit per hour"`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Change the version value of the migration script to a newer value is sufficient before next release

RemotelinkCommitShaPattern string `gorm:"type:varchar(255);comment='golang regexp, the first group will be recognized as commit sha, ref https://github.com/google/re2/wiki/Syntax'" json:"remotelinkCommitShaPattern"`
Proxy string `json:"proxy"`
RateLimit int `comment:"api request rate limt per hour" json:"rateLimit"`
RateLimitPerHour int `comment:"api request rate limt per hour" json:"rateLimit"`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For Jira, please add a new migration script to rename the column, because unlink other plugins (connection is not supported before, thus no record in the database), Jira connection exists a long time ago, we need to keep those existing records in database intact. Here is an example FYI:
https://github.com/apache/incubator-devlake/blob/release-v0.11-hotfix/plugins/gitlab/models/migrationscripts/updateSchemas20220510.go

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I found that jira tables have been dropped too

Copy link
Copy Markdown
Contributor

@abeizn abeizn Jul 14, 2022

Choose a reason for hiding this comment

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

I found that jira tables have been dropped too
@warren830
No, connction exists. zhenmian is right, but the other way is that I directly changed the ratelimit parameter, then he doesn't need to write the migration scripts.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry, I misunderstood

Copy link
Copy Markdown
Contributor

@warren830 warren830 left a comment

Choose a reason for hiding this comment

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

LGTM! Good job!

@warren830 warren830 merged commit 6fa2917 into apache:main Jul 14, 2022
@thenicetgp
Copy link
Copy Markdown
Contributor

@phzbbbbbbbb Hi, thank you for your contribution to Apache Devlake, Devlake community specially invites you to join our contributor group and issue you a contributor certificate. If you are willing, please contact 13120413800@163.com.

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.

[Refactor][plugin helper] rename connection.RateLimit to connection.RateLimitPerHour

6 participants