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

fix: use *time.Time to represent finish time #4056

Merged
merged 2 commits into from
May 26, 2023

Conversation

g1eny0ung
Copy link
Member

What problem does this PR solve?

Close #1659

What's changed and how it works?

Use *time.Time to represent the finish time, this can prevent an invalid date 0000-00-00 being inserted when creating a chaos object.

Related changes

  • This change also requires further updates to the website (e.g. docs)
  • This change also requires further updates to the UI interface
  • Need to cheery-pick to release branches
    • release-2.5
    • release-2.4

Checklist

CHANGELOG

  • I have updated the CHANGELOG.md
  • I have labeled this PR with "no-need-update-changelog"

Tests

  • Unit test
  • E2E test
  • No code
  • Manual test (add steps below)

Side effects

  • Breaking backward compatibility

DCO

If you find the DCO check fails, please run commands like below (Depends on the actual situations. For example, if the failed commit isn't the most recent) to fix it:

git commit --amend --signoff
git push --force

Signed-off-by: Yue Yang <g1enyy0ung@gmail.com>
@chaotic-prow chaotic-prow bot requested review from cwen0 and Yiyiyimu May 21, 2023 10:13
Signed-off-by: Yue Yang <g1enyy0ung@gmail.com>
@g1eny0ung g1eny0ung removed the request for review from Yiyiyimu May 21, 2023 10:15
@g1eny0ung g1eny0ung added type/bug-fix Fix for a previously reported bug. component/dashboard labels May 21, 2023
@codecov
Copy link

codecov bot commented May 21, 2023

Codecov Report

Merging #4056 (f4065da) into master (73117dc) will increase coverage by 0.15%.
The diff coverage is 14.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4056      +/-   ##
==========================================
+ Coverage   38.61%   38.77%   +0.15%     
==========================================
  Files         167      167              
  Lines       13729    13749      +20     
==========================================
+ Hits         5302     5331      +29     
+ Misses       7998     7985      -13     
- Partials      429      433       +4     
Impacted Files Coverage Δ
...ntrollers/multicluster/remotecluster/controller.go 0.00% <0.00%> (ø)
pkg/config/controller.go 0.00% <ø> (ø)
pkg/config/dashboard.go 40.62% <ø> (ø)
pkg/dashboard/core/workflow.go 41.85% <0.00%> (ø)
pkg/helm/chart.go 36.00% <26.08%> (-9.46%) ⬇️

... and 9 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update afab5b4...f4065da. Read the comment docs.

@g1eny0ung
Copy link
Member Author

/cc @cwen0

@g1eny0ung
Copy link
Member Author

/cc @STRRL

@chaotic-prow chaotic-prow bot requested a review from STRRL May 22, 2023 02:10
@STRRL
Copy link
Member

STRRL commented May 22, 2023

it might bring breaking changes on db table schema, although we enabled the AutoMigrate of gorm.

I would take a try with MySQL later, then I would give my lgtm

@g1eny0ung
Copy link
Member Author

it might bring breaking changes on db table schema, although we enabled the AutoMigrate of gorm.

I would take a try with MySQL later, then I would give my lgtm

Oh...yes, I tested in a new MySQL database and all works fine, but I didn't test on an existing MySQL with old Chaos experiments.

@STRRL
Copy link
Member

STRRL commented May 26, 2023

/lgtm

I noticed that gorm already used null for that column, no matter whether we use the pointer or not in the go struct.

create table experiments
(
    id          int unsigned auto_increment
        primary key,
    created_at  datetime      null,
    updated_at  datetime      null,
    deleted_at  datetime      null,
    uid         varchar(255)  null,
    kind        varchar(255)  null,
    name        varchar(255)  null,
    namespace   varchar(255)  null,
    action      varchar(255)  null,
    start_time  datetime      null,
    finish_time datetime      null,
    archived    tinyint(1)    null,
    experiment  varchar(4096) null
);

@chaotic-prow
Copy link

chaotic-prow bot commented May 26, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: STRRL

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@chaotic-prow chaotic-prow bot merged commit 2dc55a2 into chaos-mesh:master May 26, 2023
@g1eny0ung g1eny0ung deleted the fix/1659 branch May 26, 2023 08:28
xlgao-zju pushed a commit to xlgao-zju/chaos-mesh that referenced this pull request Jun 19, 2023
* fix: use `*time.Time` to represent finish time

Signed-off-by: Yue Yang <g1enyy0ung@gmail.com>

* Update CHANGELOG.md

Signed-off-by: Yue Yang <g1enyy0ung@gmail.com>

---------

Signed-off-by: Yue Yang <g1enyy0ung@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dashboard: can't insert experiment in MySQL NO_ZERO_IN_DATE mode
2 participants