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

Barbecue images #1803

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Barbecue images #1803

wants to merge 12 commits into from

Conversation

dottorblaster
Copy link
Contributor

@dottorblaster dottorblaster commented Sep 11, 2023

Description

Integrating Barbecue images in our CI for end to end tests and in our local development environment.

How was this tested?

End to end tests were updated to match new values and added where missing.

Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

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

Good job.
No critical blockers for this iteration.

However, besides what commented, here's a few more notes:

several checks related to corosync cmap ctl fail
error while executing corosynccmap-ctl: exec: "corosync-cmapctl": executable file not found in $PATH

package version related checks have a similar issue
error while fetching package version: package corosync is not installed exit status 1

Here /clusters/597a00db-5920-5033-b3b4-1dc98ca8718a/executions/last/373DB8/host/hana_node01 there is a failure because something can not be found in rhai
Unknown property 'value' - a getter is not registered for type '()' (line 4, position 55)
Either the check needs to cover the case or we are missing something in the fake gathered fact.

Here /clusters/597a00db-5920-5033-b3b4-1dc98ca8718a/executions/last/816815/host/hana_node01 there is a failure because
systemd gatherer not initialized properly

We need extra iterations and extra effort to get faked agent cover further scenarios and maximize their value.

'7E0221': CRITICAL,
'822E47': CRITICAL,
'845CC9': PASSING,
A1244C: CRITICAL,
Copy link
Member

Choose a reason for hiding this comment

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

Not a blocker, but what about having all check ids as string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just Prettier, I will give it a try but I think the formatter will just revert every attempt of mine 👍

@@ -17,6 +17,12 @@ export const availableClusters = [
sid: '',
type: 'Unknown',
},
{
id: 'fa0d74a3-9240-5d9e-99fa-61c4137acf81',
Copy link
Member

Choose a reason for hiding this comment

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

This is hana_cluster_2's id. It should be 597a00db-5920-5033-b3b4-1dc98ca8718a

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough

context('HANA scale-up checks', () => {
const clusterID = 'd2522281-2c76-52dc-8500-10bdf2cc6664';
context('HANA scale-up checks', () => {
const clusterID = '597a00db-5920-5033-b3b4-1dc98ca8718a';
Copy link
Member

Choose a reason for hiding this comment

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

To avoid duplication and error we could get this id from availableClusters

['DC5429', PASSING],
['F50AF5', PASSING],
];
const expectedCheckResults = {
Copy link
Member

Choose a reason for hiding this comment

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

There are 7 (if I counted correctly 😄 ) missing checks from this expectation list. Since in the test we are selecting them all it might make sense to have an expectation for all of them as well.

@@ -54,7 +54,7 @@ context('Hosts Overview', () => {
.then((i) => {
cy.get('@hostRow')
.eq(i)
.should('contain', host.agentVersion.slice(0, 15));
.should('contain', host.agentVersion.slice(0, 13));
Copy link
Member

Choose a reason for hiding this comment

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

Pls help. What did change here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agent's version is not coming from a fixture, it's the actual version of the agent, so we must limit the slice to a fewer number of chars given that the commit sha will often change

@@ -179,13 +179,15 @@ context('Hosts Overview', () => {
});

it('should change the health to warning if saptune is not installed', () => {
cy.contains('button', '2').click();
Copy link
Member

Choose a reason for hiding this comment

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

Is this about navigating to the page where the relevant host is, since we added two?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants