Skip to content

Support tracking dependencies by branch #16

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

dmiedema
Copy link
Contributor

In addition to version and all the features around that, we can also
explicitly track a remote branch that will be fetched/updated when
modulo update is run.

Add make test command
Lets make running tests easier by adding a Make command for it

Lets make running tests easier by adding a Make command for it
In addition to `version` and all the features around that, we can also
explicitly track a remote `branch` that will be fetched/updated when
`modulo update` is run.
Copy link
Contributor

@bsneed bsneed left a comment

Choose a reason for hiding this comment

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

Not really what was discussed in slack yesterday as a solution.

Command should be:

--unmanaged <branch/tag/hash/nothing>

Where, a branch/tag/hash can be specified, or simply nothing. No need to store it other than for checkout purposes.

Make for the `--unmanaged` flag on `add` to have an optional argument of
a commit or a branch. However this lead to complications around other
flags being passed such as `add blah --unmanaged -u -v`.

To resolve that issue, the properties on `Option` were move to `public`
so the add command could try its best to still apply arguments when
they were inadvertantly swallowed by the now `--unmanaged` optional
value.

Also the `unmanagedValue` is tracked in the `.modulo` file like `branch`
was but is more generic supporting an entire commit hash or a branch
name
@dmiedema
Copy link
Contributor Author

Expanding on the message in
125e682

The main weirdness is that it doesn't look like ELCLI supports an optional value. Which makes sense, that's a unique use case. It could be extended to support just that but for now, the fastest change I could do was to make the properties on Option public and attempt to reapply the swallowed flag fir this one particular use case.

Rather than have the add command hack in support for optional values to
flags make it a core feature of `ELCLI`. That seems like the proper
place to handle this situation and also doesn't break existing
functionality/assumptions in all the other uses/tests.
@johnclayton
Copy link

johnclayton commented Oct 1, 2018

I had an understanding of what this was going to be more akin to what @bsneed had. The --unmanaged <hash|branch|nothing> option would not track the value going forward but just do the initial checkout.

I also think the interface should be normalized. The add command command should be able to manage a version|hash|branch through the same interface, and unmanage with a version as well.

So:

$ modulo add --version <version|hash|branch>
$ modulo add --unmanaged <version|hash|branch|nothing>

Maybe --version is not the right place to put those other two value types but it's nice to be able to just track a branch during development, and a specific hash can really help when encountering a bug or something that requires a temporary pin to a specific commit.

@@ -17,6 +17,9 @@ public struct DependencySpec {
var repositoryURL: String
// version or version range
var version: SemverRange?
/// Optional unmanaged property to track
/// such as a branch name, commit hash, or nothing
var unmanagedValue: String?
Copy link

@cgarvin647 cgarvin647 Oct 1, 2018

Choose a reason for hiding this comment

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

Is unmanagedValue the right name for this? Maybe we could use unmanagedBranchOrHash, unmanagedCheckout. Honestly I'm not so sure I agree with the --unmanaged name option. It isn't unmanaging anything, it's more not using a version. unversioned?

Copy link

@cgarvin647 cgarvin647 left a comment

Choose a reason for hiding this comment

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

I'd like to see some clearer names being used. I like the direction of this.

@dmiedema
Copy link
Contributor Author

dmiedema commented Oct 2, 2018

Yeah, --unmanaged is probably not the flag to attach this to, and unmanagedValue really isn't the best naming choice.

Maybe --unmanaged remains exactly as is but an additional flag is added along side --version

Maybe something like --tracked or --managed and {tracked|managed}BranchOrHash for the JSON key? That kind of makes sense to me and is far more generic than my initial --branch flag

That might be more explicit and easier to understand also. It would render the changes in ECLI unnecessary (although they may be useful on their own? I'm not sure of all of its use cases) as well.

@bsneed
Copy link
Contributor

bsneed commented Oct 2, 2018

Nah, these comments are getting away from the point.

Unmanaged = You manage it yourself. Checkout whatever branch/hash/tag you desire. Optionally, it will do the initial checkout for you, nothing more.

You want it managed, you use a version. The end. No branches/hashes/non-semver tags, etc.

@dmiedema hit me up on slack if this doesn't clear it up.

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.

4 participants