Skip to content

Conversation

@jeremytchang
Copy link
Collaborator

Updated to match what is running in production

Updated to match what is running in production
@jeremytchang jeremytchang requested a review from jkaster November 5, 2021 21:06
@google-cla google-cla bot added the cla: yes label Nov 5, 2021
Copy link

@jkaster jkaster left a comment

Choose a reason for hiding this comment

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

A suggestion and a question

## Specific steps to `yarn`
1. run `yarn install` in sdk-codegen
1. run `yarn install` in sdk-codegen
2. run `yarn build` in sdk-codgen
Copy link

Choose a reason for hiding this comment

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

Suggested change
2. run `yarn build` in sdk-codgen
2. run `yarn build` in sdk-codegen

const HACKATHON_GROUP_PREFIX = 'Looker_Hack: '
const LANDING_PAGE_ATTR_NAME = 'landing_page'
const LANDING_PAGE_PATH = '/extensions/hackathon::hackathon_app'
const LANDING_PAGE_PATH = '/extensions/hackathon::hackathon'
Copy link

Choose a reason for hiding this comment

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

Is the user registration script setting this path for the default landing page? Are all existing registrants going to have their homepage updated to this new URL?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The user registration group creates a group for hackathon 2021. The script assigns a role to the group that defines the landing page. When users are added, they are added to the group and thus get the landing page attribute overwritten by the group role.
So technically, this change shouldn't be needed. And i can jsut change our manifest.kml to rename to hackathon_app. But i want to be safe.

jkaster
jkaster previously approved these changes Nov 5, 2021
Copy link

@jkaster jkaster left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@jkaster jkaster left a comment

Choose a reason for hiding this comment

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

LGTM

@jeremytchang jeremytchang merged commit 382a9f0 into main Nov 5, 2021
@jeremytchang jeremytchang deleted the jc/cleanup_hackathon branch November 5, 2021 23:07
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