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

More direct DB migration for Project and fix for group promote/demote #768

Merged
merged 2 commits into from Nov 1, 2018

Conversation

Projects
None yet
3 participants
@chefsalim
Member

chefsalim commented Nov 1, 2018

Fix a breakage in group project promote/demote and convert Project visibility to an enum

tenor-164312402

Updates to job migration
Signed-off-by: Salim Alam <salam@chef.io>
@thesentinels

This comment has been minimized.

Contributor

thesentinels commented Nov 1, 2018

Thanks for the pull request! Here is what will happen next:

  1. Your PR will be reviewed by the maintainers
  2. If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

@elliott-davis

Left some questions

@@ -11,7 +12,7 @@ table! {
owner_id -> BigInt,
created_at -> Nullable<Timestamptz>,
updated_at -> Nullable<Timestamptz>,
visibility -> Text,
visibility -> PackageVisibilityMapping,

This comment has been minimized.

@elliott-davis

elliott-davis Nov 1, 2018

Member

I don't see a migration for this? I suspect it will fail?

This comment has been minimized.

@chefsalim

chefsalim Nov 1, 2018

Member

This is copied over from packages, but I'm not seeing where it's defined either

This comment has been minimized.

@elliott-davis

elliott-davis Nov 1, 2018

Member

The DbEnum derives it. If you don't change the column type in the database to the origin_package_visibility enum, you're going to have a bad time

This comment has been minimized.

@chefsalim

chefsalim Nov 1, 2018

Member

Right, so that's in the migration

ALTER TABLE origin_projects ALTER COLUMN visibility SET DEFAULT 'public'::origin_package_visibility;

This comment has been minimized.

@elliott-davis

elliott-davis Nov 1, 2018

Member

That changes the default but doesn't alter the column type from text to origin_package_visibility or migrate any of the existing visibilities. I'm a little surprised it let you change the default to a non-text column

This comment has been minimized.

@chefsalim

chefsalim Nov 1, 2018

Member

Ah ok, will update.. it definitely works fine without the type change.. it leaves the underlying type as text.

.bind::<BigInt, _>(req.channel_id)
"select * from add_audit_package_group_entry_v2($1, $2, $3, $4, $5, $6, $7, $8)",
).bind::<Text, _>(req.origin)
.bind::<Text, _>(req.channel)

This comment has been minimized.

@elliott-davis
@@ -205,7 +205,7 @@ fn create_project((req, body): (HttpRequest<AppState>, Json<ProjectCreateReq>))
vcs_type: "git",
vcs_data: &vcs_data,
install_id: body.installation_id as i64,
visibility: &origin.default_package_visibility.to_string(),
visibility: &origin.default_package_visibility.into(),

This comment has been minimized.

@elliott-davis

elliott-davis Nov 1, 2018

Member

is the into necessary here? The origin model should return PackageVisibility by default

This comment has been minimized.

@chefsalim

chefsalim Nov 1, 2018

Member

Good catch

@@ -135,7 +135,7 @@ fn create_project((req, body): (HttpRequest<AppState>, Json<ProjectCreateReq>))
vcs_type: "git",
vcs_data: "https://github.com/habitat-sh/testapp.git",
install_id: body.installation_id as i64,
visibility: &originsrv::OriginPackageVisibility::Public.to_string(),
visibility: &originsrv::OriginPackageVisibility::Public.into(),

This comment has been minimized.

@elliott-davis

elliott-davis Nov 1, 2018

Member

maybe use PackageVisibility::Public here?

This comment has been minimized.

@chefsalim

@chefsalim chefsalim force-pushed the SA/more-migrations branch 3 times, most recently from 07717d2 to 432a8b5 Nov 1, 2018

Update project visibility type
Signed-off-by: Salim Alam <salam@chef.io>

@chefsalim chefsalim force-pushed the SA/more-migrations branch from 432a8b5 to 50860e3 Nov 1, 2018

@chefsalim chefsalim merged commit 02ebd93 into master Nov 1, 2018

2 checks passed

DCO This commit has a DCO Signed-off-by line
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@chefsalim chefsalim deleted the SA/more-migrations branch Nov 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment