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

Create a home app #1032

Merged
merged 32 commits into from
Sep 12, 2023
Merged

Create a home app #1032

merged 32 commits into from
Sep 12, 2023

Conversation

ian-noaa
Copy link
Collaborator

@ian-noaa ian-noaa commented Aug 2, 2023

Create a home app using Go & Gin with templated HTML and minimal JS.

This PR mimics the style of the original home app as much as possible. However, it updates the UI to use Bootstrap 5. I did add a favicon & a page title as well to make the MATS landing page look more official.

I've done a few additional things as they were easy to do now that we have a Go server:

  • I added a /health endpoint to the application so that we can use automated health checks in Kubernetes.
  • I added some basic Prometheus metrics so we can tell if the app is up and if it's taking longer than expected to respond to users. In the future we may be able to instrument the app links so we can tell how often certain apps are used. Otherwise, we could implement something like Google Analytics to get that info.
  • Added containerization and CI around the project so we can deploy it.

This is intended to slot into our current docker-compose stacks and replace matsapps/production:mats-http which is this Go file server: https://github.com/halverneus/static-file-server. Obviously, we could also deploy this in Kubernetes.

This PR is stacked on top of #1031 and under #1037.

Mimic the style of the original home app as much as possible. However,
update the UI to bootstrap 5.
@ian-noaa ian-noaa self-assigned this Aug 2, 2023
@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

GitHub Actions default.run.working-directory only applies to job steps
with "run" components. job steps with the "uses" keyword aren't affected
and need the working directory specified directly, if they support it.
This is probably what we want for things like "actions/checkout" but
isn't the most obvious.
Use Prometheus and some custom middleware to get information on requests
made to the app.
We now have multiple files
This was referenced Aug 2, 2023
@ian-noaa ian-noaa linked an issue Aug 2, 2023 that may be closed by this pull request
I was seeing double path proxy references which I'm suspecting were due
to using a relative href and some nginx redirect rules. Otherwise, it
may be best to just strip the proxy_prefix stuff out of here and leave
it all to nginx redirct rules to handle. I'll try this first.
This might be able to be handled purely through nginx rewrites.
We have the METexpress true/false flag already so just use that for now.
It might be wise to refactor those out into their own function to
simplify the main method at some point.
@ian-noaa ian-noaa marked this pull request as ready for review August 5, 2023 03:35
@ian-noaa
Copy link
Collaborator Author

ian-noaa commented Aug 5, 2023

@mollybsmith-noaa & @randytpierce - If you want to check it out, I've deployed a preview of the app at: https://apps-dev2.gsd.esrl.noaa.gov/mats-dev/. Note that the MET apps and the ensemble app aren't expected to work yet as they aren't deployed in the cluster. The URLs for those apps should be correctly formed though.

If you're looking through the code, feel free to ignore everything inhome/static/bootstrap-5.3.1-dist as it's code I've vendored from Bootstrap.

Some questions:

  • Is a Go app acceptable here? How do we feel about supporting it? It's a "new" language for MATS but the app itself is relatively simple and it uses the same technologies as vxDataProcessor. Go allows for quick build times and some useful instrumentation via Prometheus. I think not needing a Mongo instance is an advantage given what the home page needs to do. The biggest downside I see of not going with Meteor is that we can't reuse the header & footer from mats-common. They seem easy to replicate though and pulling in mats-common would bring a lot of dependencies that we won't use. (E.g. Couchbase) Requiring Meteor/NPM would also complicate the container image build.
  • The styling has been slightly modified from MATS - most of this is due to the move to Bootstrap 5. Are the changes acceptable?
  • Does this belong in the MATS repo? A standalone repo would be easier to reason about from a CI perspective. However, having the code in one place is advantageous for project management.

And a few known deficiencies

  • I haven't implemented testing yet
  • I should set up dependabot to update dependencies

To run this locally, you will need Go installed. Otherwise, you can follow the instructions in home/README.md.

@mollybsmith-noaa
Copy link
Collaborator

From a design standpoint, can we a) have a 1em margin at the top of the page, and b) not bold that button font? It's a bit heavy as is.
Screenshot 2023-08-16 at 1 32 10 PM

Copy link
Collaborator

@mollybsmith-noaa mollybsmith-noaa left a comment

Choose a reason for hiding this comment

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

The code all looks good, but I requested a couple of design changes in a comment. Thanks for working on this!

This matches the rest of the MATS apps but not the existing homepage.
Copy link
Collaborator

@randytpierce randytpierce left a comment

Choose a reason for hiding this comment

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

Really nice. I really like how GO handles the templates. I think the code is fine and I'll defer look and feel issues to Molly. I would like to see the buttons disable if the app cannot retrieve the version from the picker route of each app. It might also be nice to present the version in a tooltip when one hovers over the app. Makes it easy to see what the build of each app actually is. Those can be future enhancements.

@mollybsmith-noaa
Copy link
Collaborator

mollybsmith-noaa commented Aug 18, 2023 via email

Initially use radiobuttons. The toggling behavior isn't ideal here.
I'll need to fix that so that it appropriately shows/hides elements
instead of toggling them.
Additionally, switch to using bootstrap types where possible - this will
let us style elements more consistently with CSS. Additionally switch
the list of Apps from Bootstraps navpills to a more semantic set of
HTML links.
Turns out you need to use bootstrap's special CSS variables to style
everything instead of normal CSS values like "background-color", etc...
By default Bootstrap's collapse helper will toggle elements on & off.
Add some custom JS to override this behavior to show/hide elements based
on the chosen selector.
@ian-noaa
Copy link
Collaborator Author

ian-noaa commented Aug 22, 2023

Yeah, I've been pleasantly surprised with how templating works in Go. It's also been good practice for Helm Chart templating since that's Go templates combined with YAML. And I'll open some tickets for those other features!

Otherwise, does this seem okay to merge into MATS (once approved) or do we want it in its own repo?

@mollybsmith-noaa - I went ahead and updated the container image on https://apps-dev2.gsd.esrl.noaa.gov/mats-dev/ with the latest from this branch. (screenshot below) I should have updated things based on all the feedback you gave me. However, I got fancy and added some toggles to show/hide the app selectors based on the "Verification Source". The "MATS Couchbase" toggle will hide itself in a production environment. Let me know what you think!

I also switched us to use more native Bootstrap elements and moved the styling into a CSS file.

image

This commit makes a number of changes:
- Utilizes const & let in place of var
- Adds an "appgroup" class to the group headers
- Adds event listeners to expand & collapse the group headers when
  their child elements are also collapsed.
@ian-noaa
Copy link
Collaborator Author

@mollybsmith-noaa - The home app should now collapse the group headers as well when there aren't any apps to display. You can find a preview at https://apps-dev2.gsd.esrl.noaa.gov/mats-dev. (Or run it locally) Let me know what you think! You'll need to approve before I can merge.

Copy link
Collaborator

@mollybsmith-noaa mollybsmith-noaa left a comment

Choose a reason for hiding this comment

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

I like it! Out of curiosity, is there a METexpress version we could use for AWS? If not we can keep using the static template for now.

@ian-noaa
Copy link
Collaborator Author

Yup! Flip a boolean and remove the non-METexpress apps in the config file and you'll get this.

image

@mollybsmith-noaa
Copy link
Collaborator

That works!

Additionally, add myself as a default assignee for Go and GitHub Actions
updates.
Base automatically changed from cleanup-old-home-apps to development September 12, 2023 18:05
ian-noaa added a commit that referenced this pull request Sep 12, 2023
Remove the previous versions of the MATS/METexpress `home` app in
preparation for #1032.
@ian-noaa ian-noaa merged commit 2d007dc into development Sep 12, 2023
38 checks passed
@ian-noaa ian-noaa deleted the 1029-create-new-home-application branch September 12, 2023 18:38
ian-noaa added a commit that referenced this pull request Sep 12, 2023
Add some k8s manifests so that we can deploy the new home app.

I needed to add a second ingress file to handle rewriting the requests
the app receives - in essence, I'm using Nginx to rewrite requests for
`/mats-dev/` to `/`. There were some subtleties here with ingress
preferring to match requests to paths with longer lengths & the regex
capture groups Nginx rewrite-target needs artificially inflating the
char "length" of the path.

I've duplicated the changes to `gsl-dev` to the `local` setup as well.

PR stacked on top of #1032
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.

Create a home application
3 participants