Skip to content

Conversation

@josschne
Copy link
Contributor

No description provided.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed a number of packages that appeared to no longer be used. Would appreciate @gsfr or @rentzso double-checking my work.

Copy link
Member

Choose a reason for hiding this comment

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

tzlocal can go. rfc3987 is needed for some advanced jsonschema functionality.

@gsfr
Copy link
Member

gsfr commented Dec 14, 2015

I rebased this on master before running.

Is the intention for run.sh not to install the dev requirements? Or should run.sh call install.sh? Please explain how this is meant to be used.

Also, you are only locking the versions of our immediate dependencies and trusting them to correctly version their own dependencies. Is that good enough? Probably yes, but let's explicitly agree.

bin/install.sh Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Do we want a -U in these pip calls in case the requirements files are updated?

Copy link
Contributor

Choose a reason for hiding this comment

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

In my experience pip will respect TXT changes for all packages that have versions specified, which IMO should be all of them

@josschne
Copy link
Contributor Author

Yep, I am expecting packages to install their own dependencies.

Talked with @andynemzek and we'll update the run.sh to call bin/install.sh. I'll make a few updates based on your feedback to the required packages as well. Thanks!

@gsfr
Copy link
Member

gsfr commented Dec 15, 2015

Maybe I was not clear on my dependency version concern: PasteScript, for instance, does not lock the version of Paste that it requires. Hence, we're not locking the version of Paste that we require, indirectly.

@kofalt
Copy link
Contributor

kofalt commented Dec 15, 2015

I would support locking everything.

Also note that the old infra has a maintenance guide which you may want to port over.

@kofalt
Copy link
Contributor

kofalt commented Jan 8, 2016

Just rebased.

@andynemzek Wanna pick this up and decide if we should merge / close?
It's been awhile. Not sure if this works with the infra changes since then.

@andynemzek
Copy link
Contributor

@josschne did you do everything you wanted to here?

@gsfr
Copy link
Member

gsfr commented Jan 9, 2016

This is not ready. My comments have not been addressed and, critically, a required dependency has been removed.

I'd suggest making the maintenance guide that @kofalt mentions a section in the README rather than a separate document, at least until it gets more substantial.

@andynemzek
Copy link
Contributor

@gsfr @kofalt can I get a sense of priority on this? Can this wait till next week when I'm back on project?

@gsfr
Copy link
Member

gsfr commented Jan 11, 2016

Thanks, @andynemzek. Yes, this can wait a week. I'm actually hoping that @josschne will wrap this up himself.

@andynemzek
Copy link
Contributor

@gsfr ok sounds good. I will see where this is at next week then.

@gsfr
Copy link
Member

gsfr commented Jan 15, 2016

I basically just redid this with the latest versions of all packages. Also updated the package list as noted in the comments above. Added a maintenance section to the README.

@josschne: run.sh is now calling install.sh to install the dependencies. However, I still don't understand why we have two requirements files, as we seem to be installing both of them unconditionally. If that is the intention, I'd be happier with two sections in one file. I don't know what our prod infra is doing, but I'd expect it to call install.sh Please explain.

@andynemzek
Copy link
Contributor

@gsfr I had on my list to check up on this...is there anything left to do?

@gsfr
Copy link
Member

gsfr commented Jan 18, 2016

Thanks, @andynemzek. Nothing to do for you. Waiting to hear from @josschne...

@kofalt
Copy link
Contributor

kofalt commented Jan 18, 2016

I'd say let's just go ahead with this one if you're both convinced it's ready to go

@gsfr
Copy link
Member

gsfr commented Jan 19, 2016

Before this gets merged, I want to understand the intended benefit of having two requirements files or go back to one.

@josschne
Copy link
Contributor Author

Looking at this later today.

@josschne
Copy link
Contributor Author

@gsfr I think the changes you made address the concern about missing dependencies. Looked them over and they LGTM. As for the discussion on install.sh / run.sh - the separation is simply to create a separation of concerns. Prod can call install.sh, and run.sh can be for "standalone" or alternative Prod implementations. Rolling everything together wouldn't allow us to "only install, but don't run". Thoughts?

@kofalt
Copy link
Contributor

kofalt commented Jan 22, 2016

This has been out in the cold a bit too long & I'm looking to make changes that depend on it, so I'm merging this. Let's open additional tickets or PRs for further changes & discussion.

kofalt added a commit that referenced this pull request Jan 22, 2016
Clean up requirements, update infra
@kofalt kofalt merged commit 13a5f89 into master Jan 22, 2016
@kofalt kofalt deleted the infra-updates branch January 22, 2016 21:34
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.

5 participants