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(settings): missing fields in api conversion and func renames #1130

Merged
merged 10 commits into from
May 16, 2024

Conversation

plyr4
Copy link
Contributor

@plyr4 plyr4 commented May 13, 2024

created_at was returning 0, only if the value was updated via API.

also, noticed a variable type newSettingps so i made it the ol' shadow variable with an underscore

@plyr4 plyr4 requested a review from a team as a code owner May 13, 2024 19:51
Copy link

codecov bot commented May 13, 2024

Codecov Report

Attention: Patch coverage is 57.14286% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 68.06%. Comparing base (927dc71) to head (0735bd4).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1130      +/-   ##
==========================================
- Coverage   68.08%   68.06%   -0.02%     
==========================================
  Files         410      410              
  Lines       13691    13699       +8     
==========================================
+ Hits         9321     9324       +3     
- Misses       3836     3841       +5     
  Partials      534      534              
Files Coverage Δ
api/types/settings/platform.go 70.90% <57.14%> (-2.63%) ⬇️

ps.SetRepoAllowlist(_ps.GetRepoAllowlist())
ps.SetScheduleAllowlist(_ps.GetScheduleAllowlist())

ps.SetCreatedAt(_ps.GetCreatedAt())
Copy link
Member

Choose a reason for hiding this comment

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

created_at shouldn't be updateable, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no but that raises a good point this function should really be called DuplicateInPlace or something like that. i see the confusion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i made some tweaks and renamed Update to FromSettings

@plyr4
Copy link
Contributor Author

plyr4 commented May 14, 2024

added a settings.FromCLIContext that fixes missing fields in Restore

@plyr4 plyr4 changed the title fix(settings): missing timestamps in api conversion fix(settings): missing fields in api conversion and func renames May 15, 2024
@plyr4 plyr4 merged commit 4a31c1e into main May 16, 2024
12 of 14 checks passed
@plyr4 plyr4 deleted the fix/settings-timestamps branch May 16, 2024 15:17
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.

3 participants