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

add custom installer url config option #43

Merged

Conversation

bollwyvl
Copy link
Contributor

Here's the proposed approach for #42.

I didn't commit with all the built changes, as it seems harder to review... please let me know if you want those committed...

There are a few other things it might be nice to add that i ran into while working this up, but maybe not on this PR:

  • the top-level function signature is long, and i made it longer 😿
    • perhaps move the config options into an interface, e.g. IOptions
    • it would then also be possible to generate markdown docs, or even linter schema from it
      • or go the other way, and store the schema, and generate the typings from that
  • ncc is very cool!
    • requiring npm i -g ncc is kinda weird... it seems like having that locked as a devDependency might be more robust
  • formatting after building seems weird
    • formatting the markdown would be good
  • actual linting with eslint would be good
  • in the hooks, git commit . is pretty heavy, wasn't expecting that (had a _NOTES.md i was writing, didn't need that!)
  • in contributing, npm run pack isn't a thing and fails

@bollwyvl
Copy link
Contributor Author

Oh: i see that to actually run the tests, I'd have to commit my changes to the build. welp, here we go!

@bollwyvl
Copy link
Contributor Author

Ha, it didn't run: I guess i don't understand how it works!

@goanpeca
Copy link
Member

goanpeca commented May 24, 2020

Thanks for working on this @bollwyvl

the top-level function signature is long, and i made it longer 😿

Yep 🤷 or we could make it not have a top level function and use run directly?

perhaps move the config options into an interface, e.g. IOptions

Yes that could be nice

it would then also be possible to generate markdown docs, or even linter schema from it
or go the other way, and store the schema, and generate the typings from that

I have no idea about that, this was my first TS project :-p

ncc is very cool!

It is :-p

requiring npm i -g ncc is kinda weird... it seems like having that locked as a devDependency might be more robust

I think I used those instructions from github, but if you advice for it, sure :-)

formatting after building seems weird

True :-)

formatting the markdown would be good

Easy piecy

actual linting with eslint would be good

🤷

in the hooks, git commit . is pretty heavy, wasn't expecting that (had a _NOTES.md i was writing, didn't need that!)

It is, it just made my life easy :-)

in contributing, npm run pack isn't a thing and fails

That is correct, I forgot to erase it.

I will take a look at why it is failing :-)

@bollwyvl
Copy link
Contributor Author

Ooh, 😀 :

user-agent : conda/4.8.3 requests/2.23.0 PyPy/3.6.9 Linux/5.3.0-1020-azure ubuntu/18.04.4 glibc/2.27

@goanpeca
Copy link
Member

Seems to be working now :-)

@goanpeca
Copy link
Member

Looks good to me @bollwyvl awesome addition :-), I can merge, clean up the contributing and make a 1.5 release.

Anything else you want to add to this? I am happy with how it is. I will take a look at the other suggestions you made.

@bollwyvl
Copy link
Contributor Author

I replaced my hacked-in branch in the example, let it fail, and review away!

@goanpeca
Copy link
Member

I replaced my hacked-in branch in the example, let it fail, and review away!

Yes I actually need to add some tests for this (any help in setting that up is welcome!), that do not require changing the action to the current branch, I actually tried using the workflow context syntax, but it seems github expects only text and not expressions.

- '*'
push:
branches:
- '1.x'
Copy link
Member

Choose a reason for hiding this comment

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

I removed the 1.x branch so just master should be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

are you wanting a change here from me? seems like something that should be done to all of them in one fell swoop...

jobs:
example-5:
name: Ex5 Miniforge for PyPy
runs-on: 'ubuntu-latest'
Copy link
Member

Choose a reason for hiding this comment

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

Will this work on other platforms? if so, maybe we should also add them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pypy would work on osx-64, but not on win-64

Copy link
Member

Choose a reason for hiding this comment

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

Maybe then we should add a note somewhere? README

And make this runs-on: ['ubuntu-latest', 'macos....]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added in 723a49a

Copy link
Contributor Author

Choose a reason for hiding this comment

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

worked like a boss:

             user-agent : conda/4.8.3 requests/2.23.0 PyPy/3.6.9 Darwin/19.4.0 OSX/10.15.4

will put it back

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it would be hard to matrix these: constructor by default generates predictable filenames, but they can be overridden (or renamed with, e.g. a build number), so i'm more inclined to leave it verbose at this point...

Copy link
Member

Choose a reason for hiding this comment

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

Verbose is fine for this

Copy link
Member

@goanpeca goanpeca left a comment

Choose a reason for hiding this comment

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

Couple of minor changes, otherwise looks good!

@bollwyvl
Copy link
Contributor Author

Yeah, so osx ran, but didn't actually try it, and didn't fail (just warned)... i'll run it once with my branch...

@bollwyvl bollwyvl requested a review from goanpeca May 24, 2020 16:02
@goanpeca goanpeca merged commit f04c05e into conda-incubator:master May 24, 2020
@goanpeca
Copy link
Member

Thanks for this @bollwyvl

Keep testing with goanpeca/setup-miniconda@master while I make the release and let me know how it goes.

@bollwyvl bollwyvl deleted the add-config-installer-url-mk2 branch May 24, 2020 17:38
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