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

feat: First commit, adding Lilt app to Contentful Marketplace #600

Closed
wants to merge 2 commits into from

Conversation

the-julianson
Copy link

Purpose

First commit to move our Lilt extension to the Contentful Marketplace.

Approach

Follow the steps in the main README.md of the current repository.

@the-julianson the-julianson requested review from gavinmatt and a team as code owners December 14, 2023 21:17
@the-julianson the-julianson marked this pull request as draft December 14, 2023 21:22
@gavinmatt gavinmatt marked this pull request as ready for review December 18, 2023 20:17
Copy link
Contributor

@whitelisab whitelisab left a comment

Choose a reason for hiding this comment

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

Hi! The app looks great, I confirmed that the various scripts work as intended.

Given that your app is an existing app in the marketplace and we need to transfer ownership of the app definition, could you please remove the two deploy scripts for now? I will then merge in this PR, and then after we transfer the production app definition to Contentful, I will open a PR to add those scripts back in. I will also configure this app to update a CHANGELOG in a follow up PR. Let me know if you have any questions!

apps/lilt/package.json Outdated Show resolved Hide resolved
apps/lilt/package.json Outdated Show resolved Hide resolved
Copy link
Contributor

@whitelisab whitelisab left a comment

Choose a reason for hiding this comment

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

I removed the deploy scripts for now

Copy link
Contributor

@whitelisab whitelisab left a comment

Choose a reason for hiding this comment

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

@the-julianson I just noticed some environment variables in the code that are not configured, so we will need to find a way to address this to ensure that the app functions properly.

import { CloseTrimmedIcon } from '@contentful/f36-icons';
import CMClient from './contentful-management-client';

const BASE_URL = process.env.CONNECTORS_BASE_URL;
Copy link
Contributor

Choose a reason for hiding this comment

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

These environment variables (CONNECTORS_BASE_URL, HYDRA_CLIENT_ID, and HYDRA_CLIENT_STATE) were likely configured in your repo where this app was hosted previously, but are not configured here.

Copy link
Author

@the-julianson the-julianson Dec 21, 2023

Choose a reason for hiding this comment

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

@whitelisab Yes, let me get back to you with those values. We were using Kubernetes to handle the env vars.

Where are you storing the values for env vars? Is it ok If I pass those to you?

@the-julianson
Copy link
Author

@whitelisab
First, thanks for your review!

I have a couple of questions:
Something I wanted to discuss with my team is about the commit history. For this PR I just copied the whole master branch from the repo in LILT to this one, and created a PR. This has deleted all the commit history, I saw other projects also having their history initialized in the first commit. Is it possible to use REBASE or EXPORT/IMPORT strategies to maintain the commit history.

Another question, how to handle the different environments. We were using branch tagging to trigger automatic deployment to our DEV (LILT Preview) and our PROD (LILT) instances.

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.

3 participants