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

Implement --no-update-manifest flag for sync #275

Merged

Conversation

tronje
Copy link

@tronje tronje commented Jan 25, 2021

Hi! I had already opened this MR on Gitlab, but then the project was moved back here. So here I go again:

This new flag does not update the manifest before executing a sync. This
is useful if one has unstaged changes in the manifest, and does not wish
for them to be discarded by an update.

I implemented this because I needed to modify the manifest and then perform a tsrc sync, in a CI environment. I run a sed script over the manifest to update all URLs, but then tsrc sync just discards my changes, so the --no-update-manifest flag helped me. Looking forward to your feedback!

tsrc/cli/sync.py Show resolved Hide resolved
tsrc/test/cli/test_sync.py Outdated Show resolved Hide resolved
tsrc/test/cli/test_sync.py Show resolved Hide resolved
@dmerejkowsky
Copy link
Collaborator

Hi! I had already opened this MR on Gitlab, but then the project was moved back here

Oops. Sorry about that.

I needed to modify the manifest and then perform a tsrc sync, in a CI environment. I run a sed script over the manifest to update all URLs,

I'm curious about why you need to do that. Care to elaborate?

Also, I'm realizing that tsrc sync actually leads to data loss if the .tsrc/manifest repo is not clean, which I think is actually a bug - It would be much better for tsrc sync to abort I guess.

@tronje
Copy link
Author

tronje commented Jan 26, 2021

I'm curious about why you need to do that. Care to elaborate?

Okay, so the scenario is this: I want to test a project using GitLab CI. Therefore my GitLab runner needs to clone the tsrc workspace, but it does not have SSH access to any GitLab repositories. This Stackoverflow answer outlines how to clone repositories within GItLab runner by using job tokens.

So that means that I need to use different URLs for the repositories than are configured for development in my manifest.
A URL needs to be changed from this:

[email protected]/group/project.git

to this:

https://gitlab-ci-token:${CI_JOB_TOKEN}@gitlab.instance/group/project.git

And that's why I grab my manifest, run a sed script to change the URLs, and then need tsrc sync to not discard the changes in the manifest.

I can't just maintain a branch or a separate remote in my manifest for this purpose, because the CI_JOB_TOKEN is not constant; it changes for every CI job (as far as I'm aware).

@dmerejkowsky
Copy link
Collaborator

dmerejkowsky commented Jan 26, 2021

Hum. We had the same problem at Tanker and we chose to create a dedicated account with ssh keys so that we could use the same manifest. This means you have manual operations to perform on runners - and I get it if this won't work for you.

Well, I guess we can merge this - I've only minor requests now - and wait if other people come with more clever solutions...

This new flag does not update the manifest before executing a sync. This
is useful if one has unstaged changes in the manifest, and does not wish
for them to be discarded by an update.

Signed-off-by: Tronje Krabbe <[email protected]>
@tronje tronje force-pushed the feature/ignore-manifest-changes-on-sync branch from 2530122 to 15d6cfc Compare January 26, 2021 17:50
@codecov-io
Copy link

codecov-io commented Jan 26, 2021

Codecov Report

❗ No coverage uploaded for pull request base (master@25b50f0). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #275   +/-   ##
=========================================
  Coverage          ?   91.41%           
=========================================
  Files             ?       25           
  Lines             ?     1269           
  Branches          ?      142           
=========================================
  Hits              ?     1160           
  Misses            ?       84           
  Partials          ?       25           

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 25b50f0...15d6cfc. Read the comment docs.

@tronje
Copy link
Author

tronje commented Jan 26, 2021

Well, I guess we can merge this - I've only minor requests now - and wait if other people come with more clever solutions...

I'm not in too much of a rush to get this merged :)
If you like, we can discuss this some more and try to see if we can come up with a better solution together.

@dmerejkowsky
Copy link
Collaborator

dmerejkowsky commented Jan 28, 2021

I'm not in too much of a rush to get this merged :)

Well, the patch works and it does solve a problem you have so let's get this merged :)

If you wish we can continue the discussion in #279 and #278

@dmerejkowsky dmerejkowsky merged commit 6f2e0df into your-tools:master Jan 28, 2021
@tronje
Copy link
Author

tronje commented Jan 28, 2021

Cool, thanks a lot!

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