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.

Sign up for GitHub

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: show build number value in the About if present in the config #14955

Merged
merged 8 commits into from
Sep 13, 2021
Merged

feat: show build number value in the About if present in the config #14955

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0ba6d52
Added code to get the build number from the config and show the value…
cccs-joel Jun 1, 2021
c0906b1
Resolved conflict by adding both suggestions
cccs-joel Aug 6, 2021
808d25e
Merge branch 'apache:master' into feat_show_build_number
cccs-joel Aug 19, 2021
7a0dc8f
Merge branch 'apache:master' into feat_show_build_number
cccs-joel Aug 19, 2021
5e071b4
As per feedback, added a variable in the config with a default value …
cccs-joel Aug 19, 2021
1010abe
Removed trailing space, splited long line as requested
cccs-joel Aug 31, 2021
55adc03
Merge branch 'apache:master' into feat_show_build_number
cccs-joel Aug 31, 2021
6996976
Merge branch 'apache:master' into feat_show_build_number
cccs-joel Sep 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions superset-frontend/src/components/Menu/Menu.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ const mockedProps = {
locale: 'en',
version_string: '1.0.0',
version_sha: 'randomSHA',
build_number: 'randomBuildNumber',
},
settings: [
{
Expand Down Expand Up @@ -277,10 +278,10 @@ test('should render the Profile link when available', async () => {
expect(profile).toHaveAttribute('href', user_profile_url);
});

test('should render the About section and version_string or sha when available', async () => {
test('should render the About section and version_string, sha or build_number when available', async () => {
const {
data: {
navbar_right: { version_sha, version_string },
navbar_right: { version_sha, version_string, build_number },
},
} = mockedProps;

Expand All @@ -289,9 +290,11 @@ test('should render the About section and version_string or sha when available',
const about = await screen.findByText('About');
const version = await screen.findByText(`Version: ${version_string}`);
const sha = await screen.findByText(`SHA: ${version_sha}`);
const build = await screen.findByText(`Build: ${build_number}`);
expect(about).toBeInTheDocument();
expect(version).toBeInTheDocument();
expect(sha).toBeInTheDocument();
expect(build).toBeInTheDocument();
});

test('should render the Documentation link when available', async () => {
Expand Down
1 change: 1 addition & 0 deletions superset-frontend/src/components/Menu/Menu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ export interface NavBarProps {
bug_report_url?: string;
version_string?: string;
version_sha?: string;
build_number?: string;
documentation_url?: string;
languages: Languages;
show_language_picker: boolean;
Expand Down
7 changes: 6 additions & 1 deletion superset-frontend/src/components/Menu/MenuRight.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ const RightMenu = ({
</Menu.Item>
</Menu.ItemGroup>,
]}
{(navbarRight.version_string || navbarRight.version_sha) && [
{(navbarRight.version_string || navbarRight.version_sha || navbarRight.build_number) && [
<Menu.Divider key="version-info-divider" />,
<Menu.ItemGroup key="about-section" title={t('About')}>
<div className="about-section">
Expand All @@ -155,6 +155,11 @@ const RightMenu = ({
SHA: {navbarRight.version_sha}
</div>
)}
{navbarRight.build_number && (
<div css={versionInfoStyles}>
Build: {navbarRight.build_number}
</div>
)}
</div>
</Menu.ItemGroup>,
]}
Expand Down
8 changes: 8 additions & 0 deletions superset/views/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,13 @@ def menu_data() -> Dict[str, Any]:
**appbuilder.languages[lang],
"url": appbuilder.get_url_for_locale(lang),
}

build_number = ""
try:
build_number = appbuilder.app.config["BUILD_NUMBER"]
except Exception as ex:
logger.debug("BUILD_NUMBER is missing from the config", ex)
Copy link
Member

Choose a reason for hiding this comment

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

this shouldn't be necessary if you add a default value in config.py, (either empty string or None). Config values should never be undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Anything required at our end at this point?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, you need to get CI passing before we can merge


return {
"menu": menu,
"brand": {
Expand All @@ -318,6 +325,7 @@ def menu_data() -> Dict[str, Any]:
"documentation_url": appbuilder.app.config["DOCUMENTATION_URL"],
"version_string": appbuilder.app.config["VERSION_STRING"],
"version_sha": appbuilder.app.config["VERSION_SHA"],
"build_number": build_number,
"languages": languages,
"show_language_picker": len(languages.keys()) > 1,
"user_is_anonymous": g.user.is_anonymous,
Expand Down