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

Backstage image - addresses issue #1 #3

Closed
wants to merge 34 commits into from
Closed

Conversation

awsvikram
Copy link

No description provided.

@nabuskey nabuskey added the enhancement New feature or request label May 17, 2023
@nabuskey nabuskey linked an issue May 17, 2023 that may be closed by this pull request
Copy link
Contributor

@nabuskey nabuskey left a comment

Choose a reason for hiding this comment

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

Thank you very much for this!
I left some comments. I don't think we captured all the requirements in the issue as well as we should have done.


jobs:
build:
runs-on: ubuntu-latest
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we pin it to a specific version? Just in case the latest image breaks our build process for some reason. ubuntu-22.04

Copy link
Author

Choose a reason for hiding this comment

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

fixed


steps:
- name: Checkout code
uses: actions/checkout@v2
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use v3 since it's the latest version.
https://github.com/actions/checkout

Copy link
Author

Choose a reason for hiding this comment

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

changes made

Comment on lines 25 to 26
with:
ref: backstage-image
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be removed and use the default branch

Copy link
Author

Choose a reason for hiding this comment

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

I will default it to main

Comment on lines 4 to 9
workflow_dispatch:
inputs:
branch:
description: "branch to build"
required: true
default: backstage-image
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think these are needed for building from the default branch

Comment on lines 10 to 12
push:
branches:
- backstage-image
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want to build on sem ver tag push. Sorry this was not explicitly mentioned in the issue. Something like

on:
  push:
    tags:
    - 'v*.*.*'

env:
ECR_REGISTRY: ${{ steps.login-ecr.outputs.registry }}
ECR_REPOSITORY: ${{ secrets.REPO_NAME }}
IMAGE_TAG: ${{ github.sha }}
Copy link
Contributor

@nabuskey nabuskey May 17, 2023

Choose a reason for hiding this comment

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

Let's set the Docker tag to the git tag value. Something in this comment should work: https://github.com/orgs/community/discussions/26686#discussioncomment-3544088

Copy link
Contributor

Choose a reason for hiding this comment

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

can we also allow for tagging separately as part of the release? passing it via parameter or something. we can have multiple tags on the image we release.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking we use this as the GH action for sem ver tags only. Are you wanting to do it for overall cnoe release process too?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah I think it would be helpful if the CNOE release can be reflected on the docker image with a custom tag as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's create a separate workflow for releases then.

Dockerfile Outdated
@@ -0,0 +1,48 @@
# This dockerfile builds an image for the backend package.
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need a Dockerfile separate from what is here: https://github.com/cnoe-io/backstage-app/blob/main/packages/backend/Dockerfile

Copy link
Contributor

Choose a reason for hiding this comment

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

The backstage doc mentions it should be moved to the repo root.

Copy link
Contributor

Choose a reason for hiding this comment

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

really? I think the build command needs to run in the repo root but you can always build -f it and reference the Dockerfile where it is. or at least it has been what I have been doing all along and it has worked

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't care where it goes tbh, but I'd rather follow the documentation where possible.

env:
ECR_REGISTRY: ${{ steps.login-ecr.outputs.registry }}
ECR_REPOSITORY: ${{ secrets.REPO_NAME }}
IMAGE_TAG: ${{ github.sha }}
Copy link
Contributor

Choose a reason for hiding this comment

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

can we also allow for tagging separately as part of the release? passing it via parameter or something. we can have multiple tags on the image we release.

branches:
- main
tags:
- 'v0.1.2'
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be v*.*.*

env:
ECR_REGISTRY: ${{ steps.login-ecr.outputs.registry }}
ECR_REPOSITORY: ${{ secrets.REPO_NAME }}
IMAGE_TAG: ${{ github.sha }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's create a separate workflow for releases then.

Dockerfile Outdated
@@ -0,0 +1,48 @@
# This dockerfile builds an image for the backend package.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't care where it goes tbh, but I'd rather follow the documentation where possible.

Comment on lines 4 to 9
workflow_dispatch:
inputs:
branch:
description: "branch to build"
required: true
default: main
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this configures manual invocation, we probably need a tag input to make sure steps below have the information necessary to build images.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

create the updatable Backstage image
3 participants