[BUG] detect-breaking-change must be required to pass for backport changes

[jainankitk]

Describe the bug

Currently, detect-breaking-change is not required for merging the backported the changes

Related component

Build

To Reproduce

1. Go to '...'
2. Click on '....'
3. Scroll down to '....'
4. See error

Expected behavior

detect-breaking-change must be passing to avoid mistakes like #15473

Additional Details

Plugins
Please list all plugins currently enabled.

Screenshots
If applicable, add screenshots to help explain your problem.

Host/Environment (please complete the following information):

• OS: [e.g. iOS]
• Version [e.g. 22]

Additional context
Add any other context about the problem here.

[reta]

@gaiksaya @peterzhuamazon may need your help here, as always, the check for breaking changes is only run against 2.x branch but not main. Could we configure it to be mandatory for 2.x only? Thank you.

[gaiksaya]

Hi @reta I do not see this check mandatory in the required workflows (repo settings).
This workflow needs to be removed from main branch https://github.com/opensearch-project/OpenSearch/blob/2.x/.github/workflows/detect-breaking-change.yml

Edit: Removed from main branch

[reta]

This workflow needs to be removed from 2.x branch https://github.com/opensearch-project/OpenSearch/blob/2.x/.github/workflows/detect-breaking-change.yml

If we remove it, it won't be running, right? We need it to be mandatory for 2.x (but not main)

[gaiksaya]

Sorry for the confusion. I meant needs to be removed from main branch https://github.com/opensearch-project/OpenSearch/blob/main/.github/workflows/detect-breaking-change.yml
However, I see that main branch is ignored anyway in the above workflow https://github.com/opensearch-project/OpenSearch/blob/main/.github/workflows/detect-breaking-change.yml#L4-L5
So it should not be running. The PR mentioned in the issue description has 2.x base branch and the workflow did run which is correct.

[reta]

Got it, thank you, could we make it required?

[jainankitk]

@gaiksaya - Yes, the workflow is running as expected. We just need it to be required check for backport PRs to disable squash and merge in case it is failing!

[gaiksaya]

@gaiksaya - Yes, the workflow is running as expected. We just need it to be required check for backport PRs to disable squash and merge in case it is failing!

Sorry did not get you. Why would we disable for backport PRs if the base branch is 2.x?
Won't that defy the purpose of breaking changes not to be included in minor versions?

[jainankitk]

@gaiksaya - Yes, the workflow is running as expected. We just need it to be required check for backport PRs to disable squash and merge in case it is failing!

Sorry did not get you. Why would we disable for backport PRs if the base branch is 2.x? Won't that defy the purpose of breaking changes not to be included in minor versions?

@gaiksaya - Probably you misread, detect-breaking-change should disable "squash/merge option" if it fails for backport PR

[gaiksaya]

I see! Let me see if we can add rule specifically for 2.x branch

[jainankitk]

@gaiksaya - Probably below image should clarify the point me and @reta are trying to make

[peterzhuamazon]

Since @gaiksaya is already taking care of it, I will let her finish the config.
Adding the check to be required for any backport branch per branch rule should be the right setup.
I dont think we need to specifically create a rule for 2.x tho.

[gaiksaya]

It's not @peterzhuamazon Looks like if you had 2.x the * rule would remove 2.x from its list.
I added new rule for 2.x with exact same rules as * and additional detect-breaking-change as required workflow

@jainankitk @reta Please verify. Looking at #15485 looks like it is applied now.

[jainankitk]

@jainankitk @reta Please verify. Looking at #15485 looks like it is applied now.

@gaiksaya - nice, thanks!

[jainankitk]

Fixed now, closing this issue!

[kaushalmahi12]

This issue makes sense though at repo level but this is not applicable in context of the mentioned PR
The mentioned PR should not be breaking in my opinion because of the following

• The enum has been moved from one package to another package which only affects the implicit serialisation of enum objects since it depends on the fully qualified classpath.
• Now given we are not serialising the ResourceType objects it should be fine and this should not break anything

on a class level this library does the following

• Check binary compatibility: This basically checks there is no illegal changes in access modifiers, class heirarchy
• Check object serialisability: This is to ensure that all java.io.Serialisable objects do not break java object serialisability in two versions

Key classes in the library to understand it

• JApiCmpWorkerAction (Main class that starts the actual work in the run method)
• JarArchiveComparator
• JavaObjectSerializationCompatibility

here are few snaps from the gradle plugin code

private void checkJavaObjectSerializationCompatibility(List<JApiClass> jApiClasses) {
		JavaObjectSerializationCompatibility javaObjectSerializationCompatibility = new JavaObjectSerializationCompatibility();
		javaObjectSerializationCompatibility.evaluate(jApiClasses);
}


List<JApiClass> compareClassLists(JarArchiveComparatorOptions options, List<CtClass> oldClasses, List<CtClass> newClasses) {
		ClassesComparator classesComparator = new ClassesComparator(this, options);
		classesComparator.compare(oldClasses, newClasses);
		List<JApiClass> classList = classesComparator.getClasses();
		if (LOGGER.isLoggable(Level.FINE)) {
			for (JApiClass jApiClass : classList) {
				LOGGER.fine(jApiClass.toString());
			}
		}
		checkBinaryCompatibility(classList);
		checkJavaObjectSerializationCompatibility(classList);
		OutputFilter.sortClassesAndMethods(classList);
		return classList;
	}

JavaObjectSerializationCompatibility

private JApiJavaObjectSerializationCompatibility.JApiJavaObjectSerializationChangeStatus checkChanges(JApiClass jApiClass) {
		JApiJavaObjectSerializationCompatibility.JApiJavaObjectSerializationChangeStatus state = JApiJavaObjectSerializationCompatibility.JApiJavaObjectSerializationChangeStatus.SERIALIZABLE_COMPATIBLE;
		if (jApiClass.getChangeStatus() == JApiChangeStatus.REMOVED) {===========> This is causing the issue!!!!!!!!!
			return JApiJavaObjectSerializationCompatibility.JApiJavaObjectSerializationChangeStatus.SERIALIZABLE_INCOMPATIBLE_CLASS_REMOVED;
		}
		state = checkChangesForClassType(jApiClass, state);
		if (state != JApiJavaObjectSerializationCompatibility.JApiJavaObjectSerializationChangeStatus.SERIALIZABLE_COMPATIBLE) {
			return state;
		}
		state = checkChangesForSuperclass(jApiClass, state);
		if (state != JApiJavaObjectSerializationCompatibility.JApiJavaObjectSerializationChangeStatus.SERIALIZABLE_COMPATIBLE) {
			return state;
		}
		state = checkChangesForFields(jApiClass, state);
		if (state != JApiJavaObjectSerializationCompatibility.JApiJavaObjectSerializationChangeStatus.SERIALIZABLE_COMPATIBLE) {
			return state;
		}
		state = checkChangesForInterfaces(jApiClass, state);
		return state;
}

private static boolean isCtClassSerializable(JarArchiveComparatorOptions options, CtClass clazz, JarArchiveComparator jarArchiveComparator) {
		ClassPool pool = clazz.getClassPool();
		try {
			return clazz.subtypeOf(pool.get("java.io.Serializable"));
		} catch (NotFoundException e) {
			if (options.getIgnoreMissingClasses().ignoreClass(e.getMessage())) {
				return false;
			} else {
				throw JApiCmpException.forClassLoading(e, clazz.getName(), jarArchiveComparator);
			}
		}
	}

Is my understanding wrong here @jainankitk @peterzhuamazon @gaiksaya @reta ?

[reta]

Thanks a mill @gaiksaya !