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

Update WebConsoleConfiguration #31

Merged
merged 2 commits into from
Jan 15, 2018
Merged

Conversation

spadgett
Copy link
Member

@spadgett spadgett commented Jan 12, 2018

  • Add new stanza for cluster config like master and metrics URLs
  • Add new stanza for extensions config
  • Remove obsolete extension properties
  • Rename script and stylesheet properties since they're now URLs
  • Add inactivityTimeoutMinutes option

/assign @deads2k
/cc @liggitt @jwforres

@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 12, 2018
@spadgett
Copy link
Member Author

I have not added config for ClusterResourceOverrides yet

@deads2k
Copy link
Contributor

deads2k commented Jan 12, 2018

@spadgett something is red. For ease of review can you split the manual changes from the generated ones?


// LogoutURL is an optional, absolute URL to redirect web browsers to after logging out of the web console.
// If not specified, the built-in logout page is shown.
LogoutURL string `json:"logoutURL" protobuf:"bytes,3,opt,name=logoutURL"`
LogoutURL string `json:"logoutURL" protobuf:"bytes,2,opt,name=logoutURL"`
Copy link
Contributor

Choose a reason for hiding this comment

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

This and the inactivity look related to "authentication"


// ClusterConfiguration holds information the web console needs to talk to the cluster such as master public URL
// and metrics public URL.
ClusterConfiguration []ClusterConfiguration `json:"clusterConfiguration" protobuf:"bytes,4,rep,name=clusterConfiguration"`
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the console only handled one cluster.

// PublicURL is where you can find the asset server (TODO do we really need this?)
PublicURL string `json:"publicURL" protobuf:"bytes,2,opt,name=publicURL"`
// PublicURL is where you can find the web console server (TODO do we really need this?)
PublicURL string `json:"publicURL" protobuf:"bytes,1,opt,name=publicURL"`
Copy link
Contributor

Choose a reason for hiding this comment

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

under ClusterConfiguration?


// ClusterConfiguration holds information the web console needs to talk to the cluster such as master public URL and
// metrics public URL.
type ClusterConfiguration struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

You're envisioning CRO under this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes

type ExtensionsConfiguration struct {
// ExtensionScriptURLs are URLs to load as scripts when the Web Console loads. The URLs must be accessible from
// the browser.
ExtensionScriptURLs []string `json:"extensionScriptURLs" protobuf:"bytes,1,rep,name=extensionScriptURLs"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. No need for a map anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's still extension properties, but the old extensions stanza from asset config is no longer supported


// ServingInfo is the HTTP serving information for these assets
ServingInfo HTTPServingInfo `json:"servingInfo" protobuf:"bytes,5,opt,name=servingInfo"`
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: top of struct.

@spadgett
Copy link
Member Author

I need help fixing the travis error. make generate shows no changes. Could it be different version of protobuf?

@deads2k
Copy link
Contributor

deads2k commented Jan 12, 2018

I need help fixing the travis error. make generate shows no changes. Could it be different version of protobuf?

Once we get hte types pinned down, I'll pull it and generate

@spadgett spadgett force-pushed the console-config branch 2 times, most recently from acdd6cc to b2a4397 Compare January 12, 2018 14:20
@spadgett
Copy link
Member Author

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 12, 2018
// metrics public URL
type ClusterInfo struct {
// MasterPublicURL is how the web console can access the OpenShift v1 server
MasterPublicURL string `j4on:"masterPublicURL" protobuf:"bytes,1,opt,name=masterPublicURL"`
Copy link
Contributor

Choose a reason for hiding this comment

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

j4on?

Copy link
Contributor

Choose a reason for hiding this comment

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

j4on?

@sttts is there a linter of a test we have that would catch stuff like this?

Copy link
Contributor

Choose a reason for hiding this comment

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

j4on is valid tag name, isn't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

j4on is valid tag name, isn't it?

Valid tag name, but there should be a json tag for every external field that doesn't explicitly opt out.

ExtensionScripts []string `json:"extensionScripts" protobuf:"bytes,7,rep,name=extensionScripts"`
// LogoutURL is an optional, absolute URL to redirect web browsers to after logging out of the web console.
// If not specified, the built-in logout page is shown.
LogoutURL string `json:"logoutURL" protobuf:"bytes,2,opt,name=logoutURL"`
Copy link
Contributor

Choose a reason for hiding this comment

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

LogoutPublicURL ?

@spadgett spadgett force-pushed the console-config branch 2 times, most recently from 42e798d to dc349a4 Compare January 12, 2018 21:31
@spadgett
Copy link
Member Author

Updated

@spadgett
Copy link
Member Author

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 12, 2018
@spadgett
Copy link
Member Author

/hold

some debate still on the CRO flag

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 12, 2018
@spadgett
Copy link
Member Author

/hold cancel

@jwforres discussed more, and we think this is good for now. We can always revisit the CRO flags later before release since they have no install or cluster up impact.

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 12, 2018
Extensions []AssetExtensionsConfig `json:"extensions" protobuf:"bytes,10,rep,name=extensions"`
// ClusterResourceOverridesEnabled indicates that the cluster is configured for overcommit and the web console
// should hide request and limit fields that will be overridden in its editors
ClusterResourceOverridesEnabled bool `json:"clusterResourceOverridesEnabled" protobuf:"varint,6,opt,name=clusterResourceOverridesEnabled"`
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need specific values?

@deads2k
Copy link
Contributor

deads2k commented Jan 15, 2018

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 15, 2018
* Add new stanza for cluster config like master and metrics URLs
* Add new stanza for extensions config
* Remove obsolete extension properties
* Rename script and stylesheet properties since they're now URLs
* Add inactivityTimeoutMinutes option
@spadgett
Copy link
Member Author

Removed the CRO flag until we know for sure what we want.

@deads2k deads2k merged commit 3477075 into openshift:master Jan 15, 2018
@spadgett spadgett deleted the console-config branch January 15, 2018 15:05
openshift-merge-robot added a commit to openshift/origin that referenced this pull request Jan 19, 2018
Automatic merge from submit-queue.

Update cluster up for console config API changes

Adopt API changes from openshift/api#31

For now, both the new and old config need to be written to avoid breaking cluster up until origin-web-console-server is updated.

/assign @deads2k 
@jwforres FYI
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/api that referenced this pull request Feb 1, 2021
Update OLM to use UID for OG Labels
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants