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

Web apps #3

Merged
merged 49 commits into from
Feb 7, 2018
Merged

Web apps #3

merged 49 commits into from
Feb 7, 2018

Conversation

jburel
Copy link
Member

@jburel jburel commented Jan 22, 2018

Changes require to use test-infra for figure and iviewer
We will need to go a step further if we want to handle repo like https://github.com/MicronOxford/webtagging
(2 apps in one repo)

Check that
ome/omero-iviewer#155 is green
ome/omero-figure#259 is green

I will work on the README when we are happy with the change.

@jburel jburel mentioned this pull request Jan 22, 2018
6 tasks
py-setup Outdated
dir=$(cd -P -- "$(dirname -- "$0")" && pwd -P)
cd $TARGET
pip install --upgrade pip setuptools
pip install -r $dir/requirements.txt
flake8 -v .
array=( $EXCLUDE )
Copy link
Member

Choose a reason for hiding this comment

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

This makes me wonder if we don't want linting in a script. And that makes me wonder if we don't want a default file (including release and version-bump) in this repo and it gets used IFF .omeroci doesn't provide one.

Copy link
Member Author

Choose a reason for hiding this comment

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

removing from check from py-setup will be necessary since I am now having to combine 2 "dockers" => flake8 is run twice

@joshmoore
Copy link
Member

(2 apps in one repo)

I would hope that it could be solved by cloning twice.

@sbesson sbesson added this to the 0.2.0 milestone Jan 29, 2018
@jburel jburel force-pushed the web-apps branch 2 times, most recently from 36ee7fe to 5657034 Compare February 4, 2018 20:30
Copy link
Member

@joshmoore joshmoore left a comment

Choose a reason for hiding this comment

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

Along with the two NAME comments, perhaps time to push the changes to the *-docker scripts. I can do that if you'd like.

app-config Outdated
#!/bin/bash
set -e
set +u
NAME=$NAME
Copy link
Member

Choose a reason for hiding this comment

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

This line itself does nothing (other than throw an exception if the variable is not set) With the introduction of utils, can we push the NAME creation there?

Copy link
Member Author

Choose a reason for hiding this comment

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

Now that I managed to get things to work with the recent changes I will do a review of the various sections and readme.

docker Outdated


app_config() {
# TODO: add to run
Copy link
Member

Choose a reason for hiding this comment

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

If NAME detection is done in utils, then this method can be removed in favor of run app config

@jburel
Copy link
Member Author

jburel commented Feb 5, 2018

Planning to do a full review
Do we also get rid of the printf?

@joshmoore
Copy link
Member

From the individual $type-$type scripts? I would. That way it can all be controlled by run().

@joshmoore
Copy link
Member

Nice. I have some changes on top (docker cli needs some work) but I would wait until this is merged so we're all working from the same point.

@jburel
Copy link
Member Author

jburel commented Feb 6, 2018

3c20072 is required for ome/scripts to work

@jburel
Copy link
Member Author

jburel commented Feb 7, 2018

@joshmoore last 3 commits required to remove the variables as discussed and to fix ome/scripts. Everything is green now
I have not touched cli and lib since you will do some adjustments in a follow-up PR

scripts-build Outdated

cd $TARGET

pip install --upgrade pip setuptools
#pip install --upgrade pip setuptools
Copy link
Member

Choose a reason for hiding this comment

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

Did you intend to remove this?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes
let me remove the comment.
I had to move it to another file, since i now use --user to run the tests (failures in ome/scripts otherwise)

@jburel
Copy link
Member Author

jburel commented Feb 7, 2018

@joshmoore removed

@joshmoore
Copy link
Member

Thanks. LGTM. I'd say let's get this in. Happy for it to be 0.2.0 and then my follow-ups can either be 0.2.1 or 0.3.0 as needed.

@joshmoore
Copy link
Member

Discussed with J-m:

  • I'm merging & tagging
  • He's updating figure & iviewer to point to origin/master
  • Next we'll look into activating cron jobs on the other repos

@joshmoore joshmoore merged commit 6065dd9 into ome:master Feb 7, 2018
@dominikl dominikl mentioned this pull request Apr 2, 2018
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.

None yet

3 participants