diff --git a/eden/mononoke/mononoke_api/src/repo_write/create_bookmark.rs b/eden/mononoke/mononoke_api/src/repo_write/create_bookmark.rs index 8b4d636350..1689aefb56 100644 --- a/eden/mononoke/mononoke_api/src/repo_write/create_bookmark.rs +++ b/eden/mononoke/mononoke_api/src/repo_write/create_bookmark.rs @@ -5,9 +5,11 @@ * GNU General Public License version 2. */ +use std::collections::HashMap; use std::sync::Arc; use bookmarks::{BookmarkName, BookmarkUpdateReason}; +use bytes::Bytes; use metaconfig_types::BookmarkAttrs; use mononoke_types::ChangesetId; use reachabilityindex::LeastCommonAncestorsHint; @@ -21,6 +23,7 @@ impl RepoWriteContext { &self, bookmark: impl AsRef, target: ChangesetId, + pushvars: Option<&HashMap>, ) -> Result<(), MononokeError> { let bookmark = bookmark.as_ref(); self.check_method_permitted("create_bookmark")?; @@ -35,7 +38,8 @@ impl RepoWriteContext { &bookmark, target, BookmarkUpdateReason::ApiRequest, - ); + ) + .with_pushvars(pushvars); if let PermissionsModel::ServiceIdentity(service_identity) = &self.permissions_model { op = op.for_service(service_identity, &self.config().source_control_service); diff --git a/eden/mononoke/mononoke_api/src/repo_write/delete_bookmark.rs b/eden/mononoke/mononoke_api/src/repo_write/delete_bookmark.rs index c6c3ec7bb3..22488752fb 100644 --- a/eden/mononoke/mononoke_api/src/repo_write/delete_bookmark.rs +++ b/eden/mononoke/mononoke_api/src/repo_write/delete_bookmark.rs @@ -5,8 +5,11 @@ * GNU General Public License version 2. */ +use std::collections::HashMap; + use anyhow::Context; use bookmarks::{BookmarkName, BookmarkUpdateReason}; +use bytes::Bytes; use metaconfig_types::BookmarkAttrs; use mononoke_types::ChangesetId; @@ -19,6 +22,7 @@ impl RepoWriteContext { &self, bookmark: impl AsRef, old_target: Option, + pushvars: Option<&HashMap>, ) -> Result<(), MononokeError> { let bookmark = bookmark.as_ref(); self.check_method_permitted("delete_bookmark")?; @@ -46,7 +50,8 @@ impl RepoWriteContext { &bookmark, old_target, BookmarkUpdateReason::ApiRequest, - ); + ) + .with_pushvars(pushvars); if let PermissionsModel::ServiceIdentity(service_identity) = &self.permissions_model { op = op.for_service(service_identity, &self.config().source_control_service); diff --git a/eden/mononoke/mononoke_api/src/repo_write/land_stack.rs b/eden/mononoke/mononoke_api/src/repo_write/land_stack.rs index 0dddc6c8bf..f47d93f138 100644 --- a/eden/mononoke/mononoke_api/src/repo_write/land_stack.rs +++ b/eden/mononoke/mononoke_api/src/repo_write/land_stack.rs @@ -5,11 +5,12 @@ * GNU General Public License version 2. */ -use std::collections::HashSet; +use std::collections::{HashMap, HashSet}; use std::sync::Arc; use blobstore::Loadable; use bookmarks::BookmarkName; +use bytes::Bytes; use futures::compat::Stream01CompatExt; use futures::future::{self, TryFutureExt}; use futures::stream::TryStreamExt; @@ -30,6 +31,7 @@ impl RepoWriteContext { bookmark: impl AsRef, head: ChangesetId, base: ChangesetId, + pushvars: Option<&HashMap>, ) -> Result { let bookmark = bookmark.as_ref(); self.check_method_permitted("land_stack")?; @@ -80,7 +82,8 @@ impl RepoWriteContext { .await?; // Pushrebase these commits onto the bookmark. - let mut op = bookmarks_movement::PushrebaseOntoBookmarkOp::new(&bookmark, changesets); + let mut op = bookmarks_movement::PushrebaseOntoBookmarkOp::new(&bookmark, changesets) + .with_pushvars(pushvars); if let PermissionsModel::ServiceIdentity(service_identity) = &self.permissions_model { op = op.for_service(service_identity, &self.config().source_control_service); diff --git a/eden/mononoke/mononoke_api/src/repo_write/move_bookmark.rs b/eden/mononoke/mononoke_api/src/repo_write/move_bookmark.rs index 68b2205d19..d757247e36 100644 --- a/eden/mononoke/mononoke_api/src/repo_write/move_bookmark.rs +++ b/eden/mononoke/mononoke_api/src/repo_write/move_bookmark.rs @@ -5,11 +5,13 @@ * GNU General Public License version 2. */ +use std::collections::HashMap; use std::sync::Arc; use anyhow::Context; use bookmarks::{BookmarkName, BookmarkUpdateReason}; use bookmarks_movement::{BookmarkUpdatePolicy, BookmarkUpdateTargets}; +use bytes::Bytes; use metaconfig_types::BookmarkAttrs; use mononoke_types::ChangesetId; use reachabilityindex::LeastCommonAncestorsHint; @@ -25,6 +27,7 @@ impl RepoWriteContext { target: ChangesetId, old_target: Option, allow_non_fast_forward: bool, + pushvars: Option<&HashMap>, ) -> Result<(), MononokeError> { let bookmark = bookmark.as_ref(); self.check_method_permitted("move_bookmark")?; @@ -62,7 +65,8 @@ impl RepoWriteContext { BookmarkUpdatePolicy::FastForwardOnly }, BookmarkUpdateReason::ApiRequest, - ); + ) + .with_pushvars(pushvars); if let PermissionsModel::ServiceIdentity(service_identity) = &self.permissions_model { op = op.for_service(service_identity, &self.config().source_control_service); diff --git a/eden/mononoke/mononoke_api/src/test/test_repo_land_stack.rs b/eden/mononoke/mononoke_api/src/test/test_repo_land_stack.rs index b7363e6928..b3ce1a4da9 100644 --- a/eden/mononoke/mononoke_api/src/test/test_repo_land_stack.rs +++ b/eden/mononoke/mononoke_api/src/test/test_repo_land_stack.rs @@ -54,7 +54,7 @@ async fn land_stack(fb: FacebookInit) -> Result<()> { // Land G - it should be rewritten even though its parent is C. let outcome = repo - .land_stack("trunk", changesets["G"], changesets["C"]) + .land_stack("trunk", changesets["G"], changesets["C"], None) .await?; let trunk_g = repo .resolve_bookmark("trunk", BookmarkFreshness::MostRecent) @@ -67,7 +67,7 @@ async fn land_stack(fb: FacebookInit) -> Result<()> { // Land D and E, both commits should get mapped let outcome = repo - .land_stack("trunk", changesets["E"], changesets["A"]) + .land_stack("trunk", changesets["E"], changesets["A"], None) .await?; let trunk_e = repo .resolve_bookmark("trunk", BookmarkFreshness::MostRecent) @@ -87,7 +87,7 @@ async fn land_stack(fb: FacebookInit) -> Result<()> { // Land F, its parent should be the landed version of E let outcome = repo - .land_stack("trunk", changesets["F"], changesets["B"]) + .land_stack("trunk", changesets["F"], changesets["B"], None) .await?; let trunk_f = repo .resolve_bookmark("trunk", BookmarkFreshness::MostRecent) diff --git a/eden/mononoke/mononoke_api/src/test/test_repo_modify_bookmarks.rs b/eden/mononoke/mononoke_api/src/test/test_repo_modify_bookmarks.rs index 89b4e2ddcd..4e99222a99 100644 --- a/eden/mononoke/mononoke_api/src/test/test_repo_modify_bookmarks.rs +++ b/eden/mononoke/mononoke_api/src/test/test_repo_modify_bookmarks.rs @@ -51,7 +51,8 @@ async fn create_bookmark(fb: FacebookInit) -> Result<()> { let repo = repo.write().await?; // Can create public bookmarks on existing changesets (ancestors of trunk). - repo.create_bookmark("bookmark1", changesets["A"]).await?; + repo.create_bookmark("bookmark1", changesets["A"], None) + .await?; let bookmark1 = repo .resolve_bookmark("bookmark1", BookmarkFreshness::MostRecent) .await? @@ -59,7 +60,8 @@ async fn create_bookmark(fb: FacebookInit) -> Result<()> { assert_eq!(bookmark1.id(), changesets["A"]); // Can create public bookmarks on other changesets (not ancestors of trunk). - repo.create_bookmark("bookmark2", changesets["F"]).await?; + repo.create_bookmark("bookmark2", changesets["F"], None) + .await?; let bookmark2 = repo .resolve_bookmark("bookmark2", BookmarkFreshness::MostRecent) .await? @@ -67,7 +69,7 @@ async fn create_bookmark(fb: FacebookInit) -> Result<()> { assert_eq!(bookmark2.id(), changesets["F"]); // Can create scratch bookmarks. - repo.create_bookmark("scratch/bookmark3", changesets["G"]) + repo.create_bookmark("scratch/bookmark3", changesets["G"], None) .await?; let bookmark3 = repo .resolve_bookmark("scratch/bookmark3", BookmarkFreshness::MostRecent) @@ -89,7 +91,7 @@ async fn move_bookmark(fb: FacebookInit) -> Result<()> { let (repo, changesets) = init_repo(&ctx).await?; let repo = repo.write().await?; - repo.move_bookmark("trunk", changesets["E"], None, false) + repo.move_bookmark("trunk", changesets["E"], None, false, None) .await?; let trunk = repo .resolve_bookmark("trunk", BookmarkFreshness::MostRecent) @@ -100,11 +102,11 @@ async fn move_bookmark(fb: FacebookInit) -> Result<()> { // Attempt to move to a non-descendant commit without allowing // non-fast-forward moves should fail. assert!( - repo.move_bookmark("trunk", changesets["G"], None, false) + repo.move_bookmark("trunk", changesets["G"], None, false, None) .await .is_err() ); - repo.move_bookmark("trunk", changesets["G"], None, true) + repo.move_bookmark("trunk", changesets["G"], None, true, None) .await?; let trunk = repo .resolve_bookmark("trunk", BookmarkFreshness::MostRecent) @@ -144,13 +146,15 @@ async fn delete_bookmark(fb: FacebookInit) -> Result<()> { let (repo, changesets) = init_repo(&ctx).await?; let repo = repo.write().await?; - repo.create_bookmark("bookmark1", changesets["A"]).await?; - repo.create_bookmark("bookmark2", changesets["F"]).await?; - repo.create_bookmark("scratch/bookmark3", changesets["G"]) + repo.create_bookmark("bookmark1", changesets["A"], None) + .await?; + repo.create_bookmark("bookmark2", changesets["F"], None) + .await?; + repo.create_bookmark("scratch/bookmark3", changesets["G"], None) .await?; // Can delete public bookmarks. - repo.delete_bookmark("bookmark1", None).await?; + repo.delete_bookmark("bookmark1", None, None).await?; assert!( repo.resolve_bookmark("bookmark1", BookmarkFreshness::MostRecent) .await? @@ -159,7 +163,7 @@ async fn delete_bookmark(fb: FacebookInit) -> Result<()> { // Deleting a bookmark with the wrong old-target fails. assert!( - repo.delete_bookmark("bookmark2", Some(changesets["E"])) + repo.delete_bookmark("bookmark2", Some(changesets["E"]), None) .await .is_err() ); @@ -170,7 +174,7 @@ async fn delete_bookmark(fb: FacebookInit) -> Result<()> { assert_eq!(bookmark2.id(), changesets["F"]); // But with the right old-target succeeds. - repo.delete_bookmark("bookmark2", Some(changesets["F"])) + repo.delete_bookmark("bookmark2", Some(changesets["F"]), None) .await?; assert!( repo.resolve_bookmark("bookmark1", BookmarkFreshness::MostRecent) @@ -180,7 +184,7 @@ async fn delete_bookmark(fb: FacebookInit) -> Result<()> { // Can't delete scratch bookmarks. assert!( - repo.delete_bookmark("scratch/bookmark3", None) + repo.delete_bookmark("scratch/bookmark3", None, None) .await .is_err() ); diff --git a/eden/mononoke/scs_server/src/from_request.rs b/eden/mononoke/scs_server/src/from_request.rs index a3a6dd3fc6..3cb407a802 100644 --- a/eden/mononoke/scs_server/src/from_request.rs +++ b/eden/mononoke/scs_server/src/from_request.rs @@ -5,11 +5,13 @@ * GNU General Public License version 2. */ +use std::collections::{BTreeMap, HashMap}; use std::convert::{TryFrom, TryInto}; use std::fmt::{Debug, Display}; use std::ops::RangeBounds; use std::str::FromStr; +use bytes::Bytes; use chrono::{DateTime, FixedOffset, TimeZone}; use faster_hex::hex_string; use mononoke_api::specifiers::{GitSha1, Globalrev}; @@ -283,3 +285,16 @@ pub(crate) fn validate_timestamp( Some(ts) => Ok(Some(ts)), } } + +/// Convert a pushvars map from thrift's representation to the one used +/// internally in mononoke. +pub(crate) fn convert_pushvars( + pushvars: Option>>, +) -> Option> { + pushvars.map(|pushvars| { + pushvars + .into_iter() + .map(|(name, value)| (name, Bytes::from(value))) + .collect() + }) +} diff --git a/eden/mononoke/scs_server/src/methods/repo.rs b/eden/mononoke/scs_server/src/methods/repo.rs index c81b1c4b9e..33dea04122 100644 --- a/eden/mononoke/scs_server/src/methods/repo.rs +++ b/eden/mononoke/scs_server/src/methods/repo.rs @@ -29,7 +29,7 @@ use source_control as thrift; use crate::commit_id::{map_commit_identities, map_commit_identity, CommitIdExt}; use crate::errors; -use crate::from_request::{check_range_and_convert, FromRequest}; +use crate::from_request::{check_range_and_convert, convert_pushvars, FromRequest}; use crate::into_response::{AsyncIntoResponseWith, IntoResponse}; use crate::source_control_impl::SourceControlServiceImpl; @@ -450,8 +450,9 @@ impl SourceControlServiceImpl { .changeset(ChangesetSpecifier::from_request(target)?) .await? .ok_or_else(|| errors::commit_not_found(target.to_string()))?; + let pushvars = convert_pushvars(params.pushvars); - repo.create_bookmark(¶ms.bookmark, changeset.id()) + repo.create_bookmark(¶ms.bookmark, changeset.id(), pushvars.as_ref()) .await?; Ok(thrift::RepoCreateBookmarkResponse {}) } @@ -481,12 +482,14 @@ impl SourceControlServiceImpl { ), None => None, }; + let pushvars = convert_pushvars(params.pushvars); repo.move_bookmark( ¶ms.bookmark, changeset.id(), old_changeset_id, params.allow_non_fast_forward_move, + pushvars.as_ref(), ) .await?; Ok(thrift::RepoMoveBookmarkResponse {}) @@ -512,8 +515,9 @@ impl SourceControlServiceImpl { ), None => None, }; + let pushvars = convert_pushvars(params.pushvars); - repo.delete_bookmark(¶ms.bookmark, old_changeset_id) + repo.delete_bookmark(¶ms.bookmark, old_changeset_id, pushvars.as_ref()) .await?; Ok(thrift::RepoDeleteBookmarkResponse {}) } @@ -538,9 +542,10 @@ impl SourceControlServiceImpl { .changeset(ChangesetSpecifier::from_request(base)?) .await? .ok_or_else(|| errors::commit_not_found(base.to_string()))?; + let pushvars = convert_pushvars(params.pushvars); let pushrebase_outcome = repo - .land_stack(¶ms.bookmark, head.id(), base.id()) + .land_stack(¶ms.bookmark, head.id(), base.id(), pushvars.as_ref()) .await? .into_response_with(&( repo.clone(), diff --git a/eden/mononoke/tests/integration/test-scs-land-stack.t b/eden/mononoke/tests/integration/test-scs-land-stack.t index 5862870228..dd20ac2939 100644 --- a/eden/mononoke/tests/integration/test-scs-land-stack.t +++ b/eden/mononoke/tests/integration/test-scs-land-stack.t @@ -144,8 +144,8 @@ attempt to land stack to G, hooks will fail error: SourceControlService::repo_land_stack failed with RequestError { kind: RequestErrorKind::INVALID_REQUEST, reason: "hooks failed:\n limit_filesize for 6b5b61e224d26706b1eb361a3900f168042bd4f7f936c64cd91963e365aec4e9: File size limit is 10 bytes. You tried to push file glarge that is over the limit (14 bytes). See https://fburl.com/landing_big_diffs for instructions." } [1] -we don't have pushvars on the thrift interface yet, but services can bypass hooks - $ scsc land-stack -R repo --name trunk -i $G -i $A --service-id trunk-permitted-service +bypass the hook to land the stack + $ scsc land-stack -R repo --name trunk -i $G -i $A --pushvar ALLOW_LARGE_FILES=true trunk updated to: f3be859b0ddb06d8c11c2bd43edd71528ff055a7 1e82c5967442db2a0774744065e7f0f4ce810e9839897f81cc012959a783884b => a194cadd16930608adaa649035ad4c16930cbd0f 6b5b61e224d26706b1eb361a3900f168042bd4f7f936c64cd91963e365aec4e9 => f3be859b0ddb06d8c11c2bd43edd71528ff055a7 diff --git a/eden/mononoke/tests/integration/test-scs-modify-bookmarks.t b/eden/mononoke/tests/integration/test-scs-modify-bookmarks.t index 7757e43eb1..ecbc4dd9e7 100644 --- a/eden/mononoke/tests/integration/test-scs-modify-bookmarks.t +++ b/eden/mononoke/tests/integration/test-scs-modify-bookmarks.t @@ -150,8 +150,8 @@ try to move trunk over a commit that fails the hooks error: SourceControlService::repo_move_bookmark failed with RequestError { kind: RequestErrorKind::INVALID_REQUEST, reason: "hooks failed:\n limit_filesize for 88dbd25ba00277e3dfdfc642d67f2c22c75ea4f8d94f011b7f526f07b6ecc345: File size limit is 10 bytes. You tried to push file dlarge that is over the limit (14 bytes). See https://fburl.com/landing_big_diffs for instructions." } [1] -there's no way to specify pushvars at the moment, however service writes can bypass hooks - $ scsc move-bookmark -R repo --name trunk -i $E --service-id trunk-permitted-service +use a pushvar to bypass the hook + $ scsc move-bookmark -R repo --name trunk -i $E --pushvar ALLOW_LARGE_FILES=true $ wait_for_bookmark_update repo trunk $E $ scsc info -R repo -B trunk Commit: 6f95f2e47a180912e108a8dbfe9d45fc417834c3