Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Add UpdateProject functionality to Flyte control plane #114

Merged
merged 41 commits into from
Aug 19, 2020

Conversation

kosigz-lyft
Copy link
Contributor

@kosigz-lyft kosigz-lyft commented Aug 13, 2020

TL;DR

Add UpdateProject functionality to Flyte control plane.

Type

  • Bug Fix
  • Feature
  • Plugin

Issue

flyteorg/flyte#474

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

@EngHabu
Copy link
Contributor

EngHabu commented Aug 13, 2020

Can you create an issue in github.com/flyte/issues (it's public) and link all of these PRs to it?

@kosigz-lyft kosigz-lyft changed the title UpdateProject in ProjectRepo Add UpdateProject functionality to Flyte control plane Aug 14, 2020
@katrogan
Copy link
Contributor

Mind adding an integration test here: https://github.com/lyft/flyteadmin/blob/master/tests/project.go to validate the Update works to only overwrite set fields?

@flyteorg flyteorg deleted a comment from codecov-commenter Aug 18, 2020
@flyteorg flyteorg deleted a comment from codecov-commenter Aug 18, 2020
@codecov-commenter
Copy link

codecov-commenter commented Aug 18, 2020

Codecov Report

Merging #114 into master will decrease coverage by 0.18%.
The diff coverage is 16.12%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #114      +/-   ##
==========================================
- Coverage   62.50%   62.32%   -0.19%     
==========================================
  Files         104      104              
  Lines        7722     7753      +31     
==========================================
+ Hits         4827     4832       +5     
- Misses       2327     2351      +24     
- Partials      568      570       +2     
Flag Coverage Δ
#unittests 62.32% <16.12%> (-0.19%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/manager/impl/project_manager.go 66.66% <0.00%> (-26.20%) ⬇️
pkg/repositories/config/migrations.go 0.00% <0.00%> (ø)
pkg/repositories/gormimpl/project_repo.go 66.66% <0.00%> (-9.81%) ⬇️
pkg/repositories/transformers/project.go 85.18% <55.55%> (-14.82%) ⬇️

Continue to review full report at Codecov.

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

tests/project.go Outdated Show resolved Hide resolved
Copy link
Contributor

@katrogan katrogan left a comment

Choose a reason for hiding this comment

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

please squash and merge when submitting! thanks for taking up this change :)

@kosigz-lyft kosigz-lyft merged commit eb9236e into master Aug 19, 2020
@kosigz-lyft
Copy link
Contributor Author

@katrogan thanks for helping me through it! There's no way I could have shipped this feature without your guidance. 😄

@kosigz-lyft kosigz-lyft mentioned this pull request Aug 19, 2020
8 tasks
eapolinario pushed a commit that referenced this pull request Sep 6, 2023
* introduce pr to update project repo

* wip

* wip

* wip + merge

* wip + compiles woot woot

* don't return types where unused; db write

* check for writeTx error

* interface

* use get

* add first integration test

* cmts

* fix

* new integration test

* ci

* apologies for all the commits; running CI off GitHub

* nit

* api update

* update mock interface

* wip

* mock update

* lol

* bump back some dependencies

* dep

* unit test and migration

* protobuf

* should be last dep change

* revert go.mode and make compile

* mock out more things, fix white space

* wip

* wip

* gofmt

* project

* Update pkg/repositories/gormimpl/project_repo.go

Co-authored-by: Katrina Rogan <[email protected]>

* Update pkg/repositories/gormimpl/project_repo.go

Co-authored-by: Katrina Rogan <[email protected]>

* cmnts

* truncate tables before tests

* update integration test

* integration test

* :=

Co-authored-by: Konstantin Gizdarski <[email protected]>
Co-authored-by: Katrina Rogan <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants