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

Allow partial sync with multiple paths #182

Merged
merged 11 commits into from
Sep 4, 2020
Merged

Conversation

ca-johnson
Copy link
Contributor

@ca-johnson ca-johnson commented Sep 3, 2020

Resolves #164

Permit specifying a list of paths to sync, instead of limiting it to just one.

  • Add utility function list_from_env_array which can read either a single value (backwards compatibility) or an array of values from env vars like ENV_0, ENV_1 etc
  • Read sync paths via this utility
  • Update sync/unshelve to consider each path
  • Add test coverage

Testing strategy

Unit, integration, test this branch in production

@ca-johnson ca-johnson changed the title Sync multiple paths Allow partial sync with multiple paths Sep 3, 2020
* master:
  Add a unittest for syncing to label revision specifier (#173)
Comment on lines +24 to +26
sync: List of paths to sync. Defaults to entire view.
client_opts: Additional options to add to client. (e.g. allwrite)
parallel: How many threads to use for parallel sync.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Driveby, document undocumented params

"""
self.root = os.path.abspath(root or '')
self.stream = stream
self.view = self._localize_view(view or [])
self.sync_paths = sync or '//...'
self.sync_paths = sync or ['//...']
assert isinstance(self.sync_paths, list)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will never fail other than when authoring unit tests which pass the wrong type - avoids confusion when making this mistake.

Ideally we would add some types to this class instead

result = self.perforce.run_sync(
'--parallel=threads=%s' % self.parallel,
'%s%s' % (self.sync_paths, revision or ''),
*sync_files,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Expands list into params, e.g.:

l = [1, 2, 3]
print(l) => [1, 2, 3]
print(*l) => 1 2 3 # i.e. print(1, 2, 3)

python/buildkite.py Outdated Show resolved Hide resolved
@ca-johnson ca-johnson marked this pull request as ready for review September 3, 2020 16:16
@@ -16,7 +16,7 @@ configuration:
stream:
type: string
sync:
type: string
type: array
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This doesnt have any actual functional effect afaik - I think buildkite have some plans to auto-generate docs from this file

@improbable-prow-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: improbable-mattchurch
To complete the pull request process, please assign
You can assign the PR to them by writing /assign in a comment when ready.

The full list of commands accepted by this bot can be found 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

@ca-johnson ca-johnson merged commit 219aae7 into master Sep 4, 2020
@ca-johnson ca-johnson deleted the sync-multiple-paths branch September 4, 2020 14:24
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.

Allow multiple paths in partial stream sync
3 participants