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

Issue 224 Fixed - Dead code removed #241

Merged
merged 2 commits into from
Oct 19, 2022
Merged

Issue 224 Fixed - Dead code removed #241

merged 2 commits into from
Oct 19, 2022

Conversation

g7morris
Copy link
Contributor

@g7morris g7morris commented Sep 6, 2022

No description provided.

Copy link
Contributor

@nigelgbanks nigelgbanks left a comment

Choose a reason for hiding this comment

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

Just a few things remaining to be cleaned up.

ENV \
HYPERCUBE_FCREPO_URL=fcrepo/fcrepo/rest \
HYPERCUBE_LOG_LEVEL=info
ENV HYPERCUBE_LOG_LEVEL=info
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

If this isn't needed, it should be removed from the config.yaml.tmpl file as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies for delay. Removed now

Copy link
Contributor

Choose a reason for hiding this comment

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

lol, turns out it was supposed to be there. https://islandora.slack.com/archives/CM6F4C4VA/p1668431519198479

@@ -13,7 +13,7 @@ additional settings, volumes, ports, etc.
| Environment Variable | Confd Key | Default | Description |
| :------------------- | :------------------- | :----------------- | :------------------------------------------------------------------------------------------------ |
| MILLINER_DRUPAL_URL | /milliner/drupal/url | drupal:80 | Drupal URL |
| MILLINER_FCREPO_URL | /milliner/fcrepo/url | fcrepo/fcrepo/rest | Fcrepo Rest API URL |
| MILLINER_FCREPO_URL | /milliner/fcrepo/url | fcrepo:8080/fcrepo/rest | Fcrepo Rest API URL |
Copy link
Contributor

Choose a reason for hiding this comment

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

Just noticed now, but the URL variables for the microservices in the README.md files are missing the prefixed http://, so this should probably be fixed across all microservices.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, can you make sure the alignment of the tables are kept? This can be done with https://marketplace.visualstudio.com/items?itemName=yzhang.markdown-all-in-one if using VS Code, and I'm sure other auto-formatters exist for other IDEs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Roger that! Will do.

Copy link
Contributor Author

@g7morris g7morris Oct 17, 2022

Choose a reason for hiding this comment

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

Not sure if I did this right in terms of alignment. I do use that auto-formatter but it seemed a bit thick on moving things around properly. Or perhaps its operator was? 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also tested this on my local. no issues discovered

@g7morris g7morris requested a review from nigelgbanks October 17, 2022 12:17
@nigelgbanks nigelgbanks merged commit 404caed into main Oct 19, 2022
@nigelgbanks nigelgbanks deleted the issue-224 branch October 19, 2022 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants