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

Rewrite the micromamba Feature #3

Merged
merged 41 commits into from
Dec 14, 2022
Merged

Rewrite the micromamba Feature #3

merged 41 commits into from
Dec 14, 2022

Conversation

maresb
Copy link
Collaborator

@maresb maresb commented Dec 10, 2022

Hi @eitsupi, I'm really sorry it took me so long to review this.

Could you please take a look at my comments when you get the chance? (I made them inline in the script.)

Thanks so much for your patience.

src/micromamba/install.sh Outdated Show resolved Hide resolved
src/micromamba/install.sh Outdated Show resolved Hide resolved
src/micromamba/install.sh Outdated Show resolved Hide resolved
src/micromamba/install.sh Outdated Show resolved Hide resolved
@maresb
Copy link
Collaborator Author

maresb commented Dec 11, 2022

@eitsupi, what do you think of the changes I'm proposing here? Do they conform with the standard? Are they a good idea? Thanks!

@eitsupi eitsupi changed the title Add comments, questions, and tweaks Rewrite the micromamba Feature Dec 12, 2022
Copy link
Member

@eitsupi eitsupi left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, looks awesome!

There are some comments.
And, could you increment the version number to 0.1.0?
https://github.com/eitsupi/mamba-devcontainer-features/blob/12349ea7a31cba62e9a7b28392e51a0b4acc688d/src/micromamba/devcontainer-feature.json#L4

Also, could you do package installs using micromamba in the tests?

src/micromamba/devcontainer-feature.json Outdated Show resolved Hide resolved
src/micromamba/devcontainer-feature.json Outdated Show resolved Hide resolved
src/micromamba/install.sh Outdated Show resolved Hide resolved
src/micromamba/install.sh Show resolved Hide resolved
src/micromamba/devcontainer-feature.json Outdated Show resolved Hide resolved
src/micromamba/install.sh Outdated Show resolved Hide resolved
@maresb
Copy link
Collaborator Author

maresb commented Dec 12, 2022

I added tests for the addCondaForge option and a test for installing the python package via micromamba. I look forward to your responses regarding the remaining open points.

Regarding the soft version matching, I opened mamba-org/mamba#2164. I think it would be easy to implement it there, if they make it open.

src/micromamba/devcontainer-feature.json Outdated Show resolved Hide resolved
src/micromamba/install.sh Outdated Show resolved Hide resolved
src/micromamba/install.sh Show resolved Hide resolved
@eitsupi
Copy link
Member

eitsupi commented Dec 13, 2022

I could, but I'm confused about the release process. How does one know which commit corresponds to a release? I feel like we should create a tag in the repository. Would it make sense to postpone the version increment until we are ready for release? Then on the final commit, we increment the number, tag it, and execute the build workflow? Or is there a better way?

Good point. Unfortunately, at the moment there is no way to do it!
See devcontainers/features#113.

As far as I know, there are currently no Features that be managed releases by tags, and once devcontainers/action#112 is merged and GitHub Actions does this, management by tags will become commonplace.......

If this is wanted now, the maintainer can manually push the tags after the release, since the release workflow is executed manually.
https://github.com/eitsupi/mamba-devcontainer-features/blob/12349ea7a31cba62e9a7b28392e51a0b4acc688d/.github/workflows/release.yaml#L2-L3

Co-authored-by: eitsupi <50911393+eitsupi@users.noreply.github.com>
@maresb
Copy link
Collaborator Author

maresb commented Dec 13, 2022

once devcontainers/action#112 is merged and GitHub Actions does this, management by tags will become commonplace

That looks great! Can we anticipate the format of the tags? From this line

    const tag = `${type}_${id}_v${version}`;

it's not clear to me what are type and id.

@eitsupi
Copy link
Member

eitsupi commented Dec 13, 2022

That looks great! Can we anticipate the format of the tags? From this line

    const tag = `${type}_${id}_v${version}`;

it's not clear to me what are type and id.

Based on the following line, it would be something like feature_micromamba_v0.1.0.
https://github.com/devcontainers/action/blob/09709bc562da6615f791c204928d1415294c647e/src/main.ts#L66

However, this is not finalized, so it is better to use this for now and push the tag again with the new format if the format changes.

@maresb
Copy link
Collaborator Author

maresb commented Dec 14, 2022

For testing purposes, I made a release in my fork:

    "features": {
        "ghcr.io/maresb/mamba-devcontainer-features/micromamba:latest": {
            "channels": "conda-forge,defaults"
        }
    }

When you're satisfied with all the changes, could we merge this PR and then do the release in a separate commit?

Copy link
Member

@eitsupi eitsupi left a comment

Choose a reason for hiding this comment

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

I think your excellent insight and lots of work made this Feature great!
I would like to make one minor update and merge it.

src/micromamba/NOTES.md Outdated Show resolved Hide resolved
Co-authored-by: eitsupi <50911393+eitsupi@users.noreply.github.com>
src/micromamba/NOTES.md Outdated Show resolved Hide resolved
@maresb
Copy link
Collaborator Author

maresb commented Dec 14, 2022

I think your excellent insight and lots of work made this Feature great!

Yes, thank you so much for your patience and for teaching me so many things! I have learned a lot from you in this PR.

@eitsupi eitsupi merged commit ce1c86a into main Dec 14, 2022
@eitsupi eitsupi deleted the questions branch December 14, 2022 09:55
@eitsupi
Copy link
Member

eitsupi commented Dec 14, 2022

Thanks for the PR this has been really great, so let me know if you want to transfer to this repo to mamba-org.

@maresb
Copy link
Collaborator Author

maresb commented Dec 14, 2022

transfer to this repo to mamba-org

I think this is a good idea, so I will propose this on Gitter in mamba-org. But in order that people can evaluate it, could you make a package under this repo?

@eitsupi
Copy link
Member

eitsupi commented Dec 14, 2022

could you make a package under this repo?

Of course.
However, to experience the release process, why don't you push the workflow trigger after #5 is merged?

image

@maresb
Copy link
Collaborator Author

maresb commented Dec 14, 2022

Got it! Shall I merge and release right now?

@eitsupi
Copy link
Member

eitsupi commented Dec 14, 2022

Yes, please try it!

@maresb
Copy link
Collaborator Author

maresb commented Dec 14, 2022

I have proposed this in Gitter: https://gitter.im/mamba-org/Lobby?at=6399afca8bdea01368b61508

@eitsupi eitsupi mentioned this pull request Jan 3, 2023
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.

2 participants