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

Merge dev branch into master - post refactor #435

Closed
jarmoza opened this issue May 31, 2023 · 6 comments
Closed

Merge dev branch into master - post refactor #435

jarmoza opened this issue May 31, 2023 · 6 comments
Assignees
Labels
refactor Simplifying or restructuring existing code or documentation.

Comments

@jarmoza
Copy link
Contributor

jarmoza commented May 31, 2023

Now that we are complete with the store refactor, subsequent features and testing, I will no merge the development branch dev_components_talk_to_store_directly into the master branch of the annotation tool.

@jarmoza jarmoza self-assigned this May 31, 2023
@jarmoza jarmoza added type:maintenance refactor Simplifying or restructuring existing code or documentation. labels May 31, 2023
@jarmoza jarmoza moved this to Specify - Active in Neurobagel May 31, 2023
@jarmoza jarmoza moved this from Specify - Active to Backlog in Neurobagel May 31, 2023
@jarmoza jarmoza moved this from Backlog to Specify - Active in Neurobagel May 31, 2023
@surchs
Copy link
Contributor

surchs commented May 31, 2023

👍 Only comment: this may be a good candidate for pair programming - if there are any merge conflicts to be resolved, that is (but I strongly expect there will be).

@jarmoza
Copy link
Contributor Author

jarmoza commented May 31, 2023

See #436 WIP PR. So far so good actually with merges with the exception of something quirky happening on GitHub's side re: e2e testing.

Re-running now here: https://github.com/neurobagel/annotation_tool/actions/runs/5137827457

If can't resolve will reach out to channel tomorrow for 2 sets of eyes.

@surchs
Copy link
Contributor

surchs commented May 31, 2023

hhm. e2e error could be many things. seems like the dev build fails which would make e2e fail as well. what are the merge conflicts? I guess you only see those once you take the PR out of draft mode?

@surchs
Copy link
Contributor

surchs commented Jun 2, 2023

this seems clear enough to work on to me (and I think it is already worked on, no?). Either way: let's move to specify-done (or clarify the remaining things) and then get it worked on

@surchs
Copy link
Contributor

surchs commented Jun 2, 2023

Additional note on #437 (that is failing atm): I'm pretty sure the failure is because we introduced

target: "static"
in 095261b.

I made a little test to remove the "static" build target in #438 and indeed the e2e passes (sidenote: still haven't figured out how to run GH actions easily on a branch so I don't have to make a PR, I guess manual would work?).

Unfortunately just removing "static" isn't the solution either, because we need that to deploy on GH pages. I could imagine that a fix would be to temporarily override the build target (i.e. make it "not static") in the GH actions workflow for the e2e test. Or the alternative is probably to figure out how to run the e2e tests with cypress against a statically compiled version of the app - I strongly believe that the latter is going to be harder.

@jarmoza
Copy link
Contributor Author

jarmoza commented Jun 2, 2023

@surchs Yes, temp paused work on this yesterday with @rmanaem having power out. I have resumed work on this today.

@jarmoza jarmoza moved this from Specify - Active to Implement - Active in Neurobagel Jun 2, 2023
@rmanaem rmanaem closed this as completed Jun 2, 2023
@github-project-automation github-project-automation bot moved this from Implement - Active to Review - Done in Neurobagel Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Simplifying or restructuring existing code or documentation.
Projects
Archived in project
Development

No branches or pull requests

3 participants